Unsound Use of CStr::from_ptr in add_architecture_hint

[lwz23]

Description The add_architecture_hint function uses unsafe { CStr::from_ptr(hint) } to interpret a raw C string pointer (*mut c_char) as a CStr. This approach assumes that the pointer is valid, properly aligned, non-null, and null-terminated. If any of these conditions are violated, the program will invoke undefined behavior (UB). The function does not validate the hint pointer or its contents, making it unsound. https://github.com/intel/tsffs/blob/1556d0facc804e35b5696622868ea7d0c3a4b989/src/interfaces/config.rs#L30

pub fn add_architecture_hint(&mut self, cpu: *mut ConfObject, hint: *mut c_char) -> Result<()> {
    let hint = unsafe { CStr::from_ptr(hint) }.to_str()?;
    let processor_number = get_processor_number(cpu)?;
    debug!(
        self.as_conf_object(),
        "add_architecture_hint({processor_number}, {hint})"
    );
    self.architecture_hints
        .insert(processor_number, ArchitectureHint::from_str(hint)?);

    Ok(())
}

Problems: this function is a pub function, so I assume user can control the hint field, it cause some problems.

1. Unchecked Pointer Validity: The function does not verify that hint is a valid pointer. If hint is null, misaligned, or invalid, the call to CStr::from_ptr results in undefined behavior.
2. Null-Termination Requirement: CStr::from_ptr requires the string to be null-terminated. If the input pointer does not point to a null-terminated string, the function will read out of bounds, causing undefined behavior.

The function does not document or enforce safety requirements for the hint parameter, leaving it up to the caller to ensure validity. This violates Rust's safety principles and makes the function unsound.

Suggestion

1. mark this function as unsafe and provide safety doc.
2. add some check in the function body.

Additional Context: This issue arises from the unsafe handling of raw pointers and unchecked assumptions about input validity. Rust's unsafe constructs should only be used when their safety guarantees can be upheld, and all potential invalid states must be handled explicitly.

[lwz23]

same problem for https://github.com/intel/tsffs/blob/1556d0facc804e35b5696622868ea7d0c3a4b989/src/interfaces/fuzz.rs#L29 and https://github.com/intel/tsffs/blob/1556d0facc804e35b5696622868ea7d0c3a4b989/src/interfaces/fuzz.rs#L239

 pub fn repro(&mut self, testcase_file: *mut c_char) -> Result<()> {
        let simics_path = unsafe { CStr::from_ptr(testcase_file) }.to_str()?;

        let testcase_file = lookup_file(simics_path)?;

        debug!(self.as_conf_object(), "repro({})", testcase_file.display());

        let contents = read(&testcase_file).map_err(|e| {
            anyhow!(
                "Failed to read repro testcase file {}: {}",
                testcase_file.display(),
                e
            )
        })?;

        self.repro_testcase = Some(contents);

        if self.iterations > 0 {
            // We've done an iteration already, so we need to reset and run
            self.restore_initial_snapshot()?;
            self.get_and_write_testcase()?;
            self.post_timeout_event()?;

            run_alone(|| {
                continue_simulation(0)?;
                Ok(())
            })?;
        }

        Ok(())
    }

pub fn solution(&mut self, id: u64, message: *mut c_char) -> Result<()> {
        let message = unsafe { CStr::from_ptr(message) }.to_str()?;

        debug!(self.as_conf_object(), "solution({id:#x}, {message})");

        self.stop_simulation(StopReason::Solution {
            kind: SolutionKind::Manual,
        })?;

        Ok(())
    }

[lwz23]

ping?

[Wenzel]

Hi @lwz23, Thanks for this bug report ! I can have a look at it in 2 weeks.

[lwz23]

Thanks for your reply. I go through those code again and find all those from_ptr result are handled by ?, so, in suppose there will be a panic instead of abort if arg is invalid. in short, I think maybe those functions just need safety document but won't trigger UB. thanks.