[Unsound] Slice created from unaligned pointer

[shinmao]

Hi, I am the security researcher from SunLab. I am testing our personal tools on open-source repositories and find the following unsound implemenation.

The source of unsoundness

pprof-rs/src/collector.rs

Lines 223 to 225 in 4939f73

	let length = self.file_vec.len() / std::mem::size_of::<T>();
	let ts =
	unsafe { std::slice::from_raw_parts(self.file_vec.as_ptr() as *const T, length) };

The function would cast file_vec which is aligned to 1 byte to the specified type, and the function can be called by try_iter() on Collector. The specified type can be decided the items in Collector. In other words, if we add the element which is aligned to more than 1 byte to the Collector, the unaligned pointer would be created and used to build the slice, which breaks the safety guarantee of slice::from_raw_parts.

This is how it could work:

fn main() {
    let a: u16 = 1;     // aligned to 2 bytes
    let mut c = Collector::new().unwrap();
    let _ = c.add(a, 1isize);
    c.try_iter().unwrap().for_each(|entry| {
        println!("{:?}", entry);
    });
}

try_iter() will call next with T as u16.

[shinmao]

We consider that the public safe api addr_validate::validate could also have the unsoundness:

pprof-rs/src/addr_validate.rs

Lines 91 to 96 in 4939f73

	let write_fd = MEM_VALIDATE_PIPE.write_fd.load(Ordering::SeqCst);
	loop {
	let buf = unsafe { std::slice::from_raw_parts(addr as *const u8, CHECK_LENGTH) };
	match write(write_fd, buf) {
	Ok(bytes) => break bytes > 0,

This function allows casting c_void input to u8 pointer. However, based on the safety guarantee of from_raw_parts, caller should make sure the pointer is initialized for consecutive length. Here, we could cast arbitrary types to c_void and pass to this function which will make uninitialized memory exposure.

This is how it works:

use core::ffi::c_void;
use pprof::validate;

#[repr(align(256))]
#[derive(Copy, Clone, Debug)]
struct Padding {
    a: u8,
    b: u32,
    c: u8,
}

fn main() {
    let pd = Padding { a: 10, b: 11, c: 12 };
    println!("{}", validate(&pd as *const Padding as *const c_void));
}

[jdsaund]

We're also experience this issue on macos m2. The PR solves it, will it get merged soon?

[finnbear]

After updating to a recent nightly compiler, I observed the following related panic on pprof@0.12.0 (debug build) as a result:

thread 'main' panicked at library/core/src/panicking.rs:215:5:
unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`
stack backtrace:
   0: rust_begin_unwind
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/panicking.rs:110:18
   2: core::panicking::panic_nounwind_fmt
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/panicking.rs:120:5
   3: core::panicking::panic_nounwind
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/panicking.rs:215:5
   4: core::slice::raw::from_raw_parts::precondition_check
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/ub_checks.rs:66:21
   5: core::slice::raw::from_raw_parts
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/slice/raw.rs:96:9
   6: <pprof::collector::TempFdArrayIterator<T> as core::iter::traits::iterator::Iterator>::next
             at /home/finnb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pprof-0.12.0/src/collector.rs:225:26
   7: core::iter::traits::iterator::Iterator::fold
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/iter/traits/iterator.rs:2586:29
   8: <core::iter::adapters::chain::Chain<A,B> as core::iter::traits::iterator::Iterator>::fold
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/iter/adapters/chain.rs:93:19
   9: core::iter::traits::iterator::Iterator::for_each
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/iter/traits/iterator.rs:817:9
  10: pprof::report::ReportBuilder::build
             at /home/finnb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pprof-0.12.0/src/report.rs:110:17

Edit: The linked PR resolved the issue! ❤️

[markmandel]

Just ran into this issue on upgrade to Rust 1.78.0. We can't upgrade beyond 1.77.x at this time with this bug in place.

You can see it failing here: https://github.com/googleforgames/quilkin/actions/runs/9375621368/job/25814057518?pr=974 (since writing I've likely downgraded this update back to 1.77.1)

[vorporeal]

Hi @YangKeao - this crate is currently broken for many users due to this (note that #250 is another report of this issue).

Can you take a look at PR #241? Without it, users of this crate are going to have to start forking it in order to have working pprof for their applications.

[markmandel]

Travelling at the moment, but anyone tried engaging on their Slack to see if we can get traction for this fix?

[plugwash]

Hi @YangKeao - this crate is currently broken for many users due to this (note that #250 is another report of this issue).

Afaict there are two seperate soundness issues, the one in collector.rs mentioned in the first post of this issue, and the one in addr_validate.rs mentioned in the second post of this issue and in issue #250.

It is the latter that is causing the testsuite to fail with recent rustc.

[shinmao]

Hi, in this way, does it mean that any use of guard.report().build() will trigger panic in the program? Or panic will only be triggered in specific environment by core::slice::raw::from_raw_parts::precondition_check?

let guard = pprof::ProfilerGuardBuilder::default()
    .frequency(self.frequency)
    .blocklist(&["libc", "libgcc", "pthread", "vdso"])
    .build()
    .context(CreateGuardSnafu)?;
// ...
guard.report().build().context(CreateReportSnafu)