-
Notifications
You must be signed in to change notification settings - Fork 235
Implement Memory Sanitizer unpoisoning more precisely. #678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
//! Implementation for Linux / Android with `/dev/urandom` fallback | ||
use super::use_file; | ||
use super::{sanitizer, use_file}; | ||
use crate::Error; | ||
use core::{ | ||
ffi::c_void, | ||
|
@@ -95,7 +95,9 @@ pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |
// note: `transmute` is currently the only way to convert a pointer into a function reference | ||
let getrandom_fn = unsafe { transmute::<NonNull<c_void>, GetRandomFn>(fptr) }; | ||
util_libc::sys_fill_exact(dest, |buf| unsafe { | ||
getrandom_fn(buf.as_mut_ptr().cast(), buf.len(), 0) | ||
let ret = getrandom_fn(buf.as_mut_ptr().cast(), buf.len(), 0); | ||
sanitizer::unpoison_linux_getrandom_result(buf, ret); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, this ends up calling |
||
ret | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
use core::mem::MaybeUninit; | ||
|
||
/// Unpoisons `buf` if MSAN support is enabled. | ||
/// | ||
/// Most backends do not need to unpoison their output. Rust language- and | ||
/// library- provided functionality unpoisons automatically. Similarly, libc | ||
/// either natively supports MSAN and/or MSAN hooks libc-provided functions | ||
/// to unpoison outputs on success. Only when all of these things are | ||
/// bypassed do we need to do it ourselves. | ||
/// | ||
/// The call to unpoison should be done as close to the write as possible. | ||
/// For example, if the backend partially fills the output buffer in chunks, | ||
/// each chunk should be unpoisoned individually. This way, the correctness of | ||
/// the chunking logic can be validated (in part) using MSAN. | ||
pub unsafe fn unpoison(buf: &mut [MaybeUninit<u8>]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be private? Do we have any existing places where we call this helper? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be private but it is intended to be useful for any future backends that need to do unpoisoning themselves. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some history for this: Originally I wrote the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, I think these are fine to have as-is then. |
||
cfg_if! { | ||
if #[cfg(getrandom_msan)] { | ||
extern "C" { | ||
fn __msan_unpoison(a: *mut core::ffi::c_void, size: usize); | ||
} | ||
let a = buf.as_mut_ptr().cast(); | ||
let size = buf.len(); | ||
#[allow(unused_unsafe)] // TODO(MSRV 1.65): Remove this. | ||
unsafe { | ||
__msan_unpoison(a, size); | ||
} | ||
} else { | ||
let _ = buf; | ||
} | ||
} | ||
} | ||
|
||
/// Interprets the result of the `getrandom` syscall of Linux, unpoisoning any | ||
/// written part of `buf`. | ||
/// | ||
/// `buf` must be the output buffer that was originally passed to the `getrandom` | ||
/// syscall. | ||
/// | ||
/// `ret` must be the result returned by `getrandom`. If `ret` is negative or | ||
/// larger than the length of `buf` then nothing is done. | ||
/// | ||
/// Memory Sanitizer only intercepts `getrandom` on this condition (from its | ||
/// source code): | ||
/// ```c | ||
/// #define SANITIZER_INTERCEPT_GETRANDOM \ | ||
/// ((SI_LINUX && __GLIBC_PREREQ(2, 25)) || SI_FREEBSD || SI_SOLARIS) | ||
/// ``` | ||
/// So, effectively, we have to assume that it is never intercepted on Linux. | ||
#[cfg(any(target_os = "android", target_os = "linux"))] | ||
pub unsafe fn unpoison_linux_getrandom_result(buf: &mut [MaybeUninit<u8>], ret: isize) { | ||
if let Ok(bytes_written) = usize::try_from(ret) { | ||
if let Some(written) = buf.get_mut(..bytes_written) { | ||
#[allow(unused_unsafe)] // TODO(MSRV 1.65): Remove this. | ||
unsafe { | ||
unpoison(written) | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is unpoisoning needed if we are calling the
libc
version ofgetrandom
? The comment insanitizer.rs
seems to indicate that thelibc
calls should handle unpoisioning.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See
getrandom/src/backends/sanitizer.rs
Lines 42 to 48 in 5f7a910
It is possible that the libc has interception built-in, but we have no way of knowing. I think common open source implementations like musl libc don't, except maybe LLVM libc? If we have a way to detect that LLVM libc will be used, we could make an exception for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Maybe Android bionic has built-in support for unpoisoning, in which case we could avoid unpoisoning on Android. IDK for sure.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused mostly because
libc::getrandom
will only exist on GLibc 2.25 or later, so I figured we would either have:/dev/urandom
or fail depending on the backendlibc::getrandom
exists and gets properly handled by msan/libcBut if there's an extra level of config between Rust libc and msan, it totally makes sense why this wouldn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josephlr Do you know what the Android bionic situation is? IDK if MSAN even works for Android. Would love to minimize the use of this as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have personally not ever used bionic with MSAN. When fuzzing or running tests against C libraries, we usually just run them on Linux, as most of the validation is not for OS specific stuff. But that doesn't mean it's not possible, I've asked some folks at work and we'll see if they get back to me.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldnt find any mention about MSan in Android's memory debugging guide: https://developer.android.com/ndk/guides/memory-debug and all the other tools I found were also of an ASan like flavor.
I would be fine assuming that:
libc::read
,libc::getrandom
, etc...) unconditionally, as newer versions of the libc will always havelibc::getrandom
I might be wrong, but it seems fine to remove this for android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about MUSL targets?