| Message ID | 20240415-alice-mm-v5-3-6f55e4d8ef51@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory management patches needed by Rust Binder | expand |
On Mon, Apr 15, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote: > > Add safe methods for reading and writing Rust values to and from > userspace pointers. > > The C methods for copying to/from userspace use a function called > `check_object_size` to verify that the kernel pointer is not dangling. > However, this check is skipped when the length is a compile-time > constant, with the assumption that such cases trivially have a correct > kernel pointer. > > In this patch, we apply the same optimization to the typed accessors. > For both methods, the size of the operation is known at compile time to > be size_of of the type being read or written. Since the C side doesn't > provide a variant that skips only this check, we create custom helpers > for this purpose. > > The majority of reads and writes to userspace pointers in the Rust > Binder driver uses these accessor methods. Benchmarking has found that > skipping the `check_object_size` check makes a big difference for the > cases being skipped here. (And that the check doesn't make a difference > for the cases that use the raw read/write methods.) > > This code is based on something that was originally written by Wedson on > the old rust branch. It was modified by Alice to skip the > `check_object_size` check, and to update various comments, including the > notes about kernel pointers in `WritableToBytes`. > > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Alice Ryhl <aliceryhl@google.com> Couple of docs nits but this looks good to me. Reviewed-by: Trevor Gross <tmgross@umich.edu> > +/// Types for which any bit pattern is valid. > +/// > +/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so > +/// reading arbitrary bytes into something that contains a `bool` is not okay. > +/// > +/// It's okay for the type to have padding, as initializing those bytes has no effect. > +/// > +/// # Safety > +/// > +/// All bit-patterns must be valid for this type. > +pub unsafe trait FromBytes {} No `UnsafeCell` is also a requirement in zerocopy/bytemuck > +/// Types that can be viewed as an immutable slice of initialized bytes. > +/// > +/// If a struct implements this trait, then it is okay to copy it byte-for-byte to userspace. This > +/// means that it should not have any padding, as padding bytes are uninitialized. Reading > +/// uninitialized memory is not just undefined behavior, it may even lead to leaking sensitive > +/// information on the stack to userspace. > +/// > +/// The struct should also not hold kernel pointers, as kernel pointer addresses are also considered > +/// sensitive. However, leaking kernel pointers is not considered undefined behavior by Rust, so > +/// this is a correctness requirement, but not a safety requirement. I don't think mentions of userspace are relevant here since the trait is more general. Maybe a `# Interfacing with userspace` section if there is enough relevant information. > +/// # Safety > +/// > +/// Values of this type may not contain any uninitialized bytes. No UnsafeCell > +pub unsafe trait AsBytes {} > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > index c97029cdeba1..e3953eec61a3 100644 > --- a/rust/kernel/uaccess.rs > +++ b/rust/kernel/uaccess.rs > @@ -4,10 +4,15 @@ > //! > //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h) > > -use crate::{bindings, error::code::*, error::Result}; > +use crate::{ > + bindings, > + error::code::*, > + error::Result, > + types::{AsBytes, FromBytes}, > +}; > use alloc::vec::Vec; > use core::ffi::{c_ulong, c_void}; > -use core::mem::MaybeUninit; > +use core::mem::{size_of, MaybeUninit}; > > /// A pointer to an area in userspace memory, which can be either read-only or read-write. > /// > @@ -238,6 +243,38 @@ pub fn read_slice(&mut self, out: &mut [u8]) -> Result { > self.read_raw(out) > } > > + /// Reads a value of the specified type. > + /// > + /// Fails with `EFAULT` if the read encounters a page fault. > + pub fn read<T: FromBytes>(&mut self) -> Result<T> { > [...] > + /// Writes the provided Rust value to this userspace pointer. > + /// > + /// Fails with `EFAULT` if the write encounters a page fault. > + pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { Read & write could use an example if you are up for it
Trevor Gross <tmgross@umich.edu> writes: > On Mon, Apr 15, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote: >> >> Add safe methods for reading and writing Rust values to and from >> userspace pointers. >> >> The C methods for copying to/from userspace use a function called >> `check_object_size` to verify that the kernel pointer is not dangling. >> However, this check is skipped when the length is a compile-time >> constant, with the assumption that such cases trivially have a correct >> kernel pointer. >> >> In this patch, we apply the same optimization to the typed accessors. >> For both methods, the size of the operation is known at compile time to >> be size_of of the type being read or written. Since the C side doesn't >> provide a variant that skips only this check, we create custom helpers >> for this purpose. >> >> The majority of reads and writes to userspace pointers in the Rust >> Binder driver uses these accessor methods. Benchmarking has found that >> skipping the `check_object_size` check makes a big difference for the >> cases being skipped here. (And that the check doesn't make a difference >> for the cases that use the raw read/write methods.) >> >> This code is based on something that was originally written by Wedson on >> the old rust branch. It was modified by Alice to skip the >> `check_object_size` check, and to update various comments, including the >> notes about kernel pointers in `WritableToBytes`. >> >> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> >> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> >> Reviewed-by: Benno Lossin <benno.lossin@proton.me> >> Reviewed-by: Boqun Feng <boqun.feng@gmail.com> >> Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > Couple of docs nits but this looks good to me. > > Reviewed-by: Trevor Gross <tmgross@umich.edu> Thanks for taking a look! >> +/// Types for which any bit pattern is valid. >> +/// >> +/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so >> +/// reading arbitrary bytes into something that contains a `bool` is not okay. >> +/// >> +/// It's okay for the type to have padding, as initializing those bytes has no effect. >> +/// >> +/// # Safety >> +/// >> +/// All bit-patterns must be valid for this type. >> +pub unsafe trait FromBytes {} > > No `UnsafeCell` is also a requirement in zerocopy/bytemuck I can add that requirement. >> +/// Types that can be viewed as an immutable slice of initialized bytes. >> +/// >> +/// If a struct implements this trait, then it is okay to copy it byte-for-byte to userspace. This >> +/// means that it should not have any padding, as padding bytes are uninitialized. Reading >> +/// uninitialized memory is not just undefined behavior, it may even lead to leaking sensitive >> +/// information on the stack to userspace. >> +/// >> +/// The struct should also not hold kernel pointers, as kernel pointer addresses are also considered >> +/// sensitive. However, leaking kernel pointers is not considered undefined behavior by Rust, so >> +/// this is a correctness requirement, but not a safety requirement. > > I don't think mentions of userspace are relevant here since the trait > is more general. Maybe a `# Interfacing with userspace` section if > there is enough relevant information. I think it is relevant. It is the main purpose of the trait right now, and it is also part of the justification for why the rules are what they are. >> +/// # Safety >> +/// >> +/// Values of this type may not contain any uninitialized bytes. > > No UnsafeCell Will add. >> +pub unsafe trait AsBytes {} > >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs >> index c97029cdeba1..e3953eec61a3 100644 >> --- a/rust/kernel/uaccess.rs >> +++ b/rust/kernel/uaccess.rs >> @@ -4,10 +4,15 @@ >> //! >> //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h) >> >> -use crate::{bindings, error::code::*, error::Result}; >> +use crate::{ >> + bindings, >> + error::code::*, >> + error::Result, >> + types::{AsBytes, FromBytes}, >> +}; >> use alloc::vec::Vec; >> use core::ffi::{c_ulong, c_void}; >> -use core::mem::MaybeUninit; >> +use core::mem::{size_of, MaybeUninit}; >> >> /// A pointer to an area in userspace memory, which can be either read-only or read-write. >> /// >> @@ -238,6 +243,38 @@ pub fn read_slice(&mut self, out: &mut [u8]) -> Result { >> self.read_raw(out) >> } >> >> + /// Reads a value of the specified type. >> + /// >> + /// Fails with `EFAULT` if the read encounters a page fault. >> + pub fn read<T: FromBytes>(&mut self) -> Result<T> { >> [...] >> + /// Writes the provided Rust value to this userspace pointer. >> + /// >> + /// Fails with `EFAULT` if the write encounters a page fault. >> + pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { > > Read & write could use an example if you are up for it I may or may not add an example. Alice
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index aa77bad9bce4..414ba602fc5b 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -409,3 +409,66 @@ pub enum Either<L, R> { /// Constructs an instance of [`Either`] containing a value of type `R`. Right(R), } + +/// Types for which any bit pattern is valid. +/// +/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so +/// reading arbitrary bytes into something that contains a `bool` is not okay. +/// +/// It's okay for the type to have padding, as initializing those bytes has no effect. +/// +/// # Safety +/// +/// All bit-patterns must be valid for this type. +pub unsafe trait FromBytes {} + +// SAFETY: All bit patterns are acceptable values of the types below. +unsafe impl FromBytes for u8 {} +unsafe impl FromBytes for u16 {} +unsafe impl FromBytes for u32 {} +unsafe impl FromBytes for u64 {} +unsafe impl FromBytes for usize {} +unsafe impl FromBytes for i8 {} +unsafe impl FromBytes for i16 {} +unsafe impl FromBytes for i32 {} +unsafe impl FromBytes for i64 {} +unsafe impl FromBytes for isize {} +// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit +// patterns are also acceptable for arrays of that type. +unsafe impl<T: FromBytes> FromBytes for [T] {} +unsafe impl<T: FromBytes, const N: usize> FromBytes for [T; N] {} + +/// Types that can be viewed as an immutable slice of initialized bytes. +/// +/// If a struct implements this trait, then it is okay to copy it byte-for-byte to userspace. This +/// means that it should not have any padding, as padding bytes are uninitialized. Reading +/// uninitialized memory is not just undefined behavior, it may even lead to leaking sensitive +/// information on the stack to userspace. +/// +/// The struct should also not hold kernel pointers, as kernel pointer addresses are also considered +/// sensitive. However, leaking kernel pointers is not considered undefined behavior by Rust, so +/// this is a correctness requirement, but not a safety requirement. +/// +/// # Safety +/// +/// Values of this type may not contain any uninitialized bytes. +pub unsafe trait AsBytes {} + +// SAFETY: Instances of the following types have no uninitialized portions. +unsafe impl AsBytes for u8 {} +unsafe impl AsBytes for u16 {} +unsafe impl AsBytes for u32 {} +unsafe impl AsBytes for u64 {} +unsafe impl AsBytes for usize {} +unsafe impl AsBytes for i8 {} +unsafe impl AsBytes for i16 {} +unsafe impl AsBytes for i32 {} +unsafe impl AsBytes for i64 {} +unsafe impl AsBytes for isize {} +unsafe impl AsBytes for bool {} +unsafe impl AsBytes for char {} +unsafe impl AsBytes for str {} +// SAFETY: If individual values in an array have no uninitialized portions, then the array itself +// does not have any uninitialized portions either. +unsafe impl<T: AsBytes> AsBytes for [T] {} +unsafe impl<T: AsBytes, const N: usize> AsBytes for [T; N] {} diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index c97029cdeba1..e3953eec61a3 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -4,10 +4,15 @@ //! //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h) -use crate::{bindings, error::code::*, error::Result}; +use crate::{ + bindings, + error::code::*, + error::Result, + types::{AsBytes, FromBytes}, +}; use alloc::vec::Vec; use core::ffi::{c_ulong, c_void}; -use core::mem::MaybeUninit; +use core::mem::{size_of, MaybeUninit}; /// A pointer to an area in userspace memory, which can be either read-only or read-write. /// @@ -238,6 +243,38 @@ pub fn read_slice(&mut self, out: &mut [u8]) -> Result { self.read_raw(out) } + /// Reads a value of the specified type. + /// + /// Fails with `EFAULT` if the read encounters a page fault. + pub fn read<T: FromBytes>(&mut self) -> Result<T> { + let len = size_of::<T>(); + if len > self.length { + return Err(EFAULT); + } + let Ok(len_ulong) = c_ulong::try_from(len) else { + return Err(EFAULT); + }; + let mut out: MaybeUninit<T> = MaybeUninit::uninit(); + // SAFETY: The local variable `out` is valid for writing `size_of::<T>()` bytes. + // + // By using the _copy_from_user variant, we skip the check_object_size check that verifies + // the kernel pointer. This mirrors the logic on the C side that skips the check when the + // length is a compile-time constant. + let res = unsafe { + bindings::_copy_from_user(out.as_mut_ptr().cast::<c_void>(), self.ptr, len_ulong) + }; + if res != 0 { + return Err(EFAULT); + } + // Since this is not a pointer to a valid object in our program, we cannot use `add`, which + // has C-style rules for defined behavior. + self.ptr = self.ptr.wrapping_byte_add(len); + self.length -= len; + // SAFETY: The read above has initialized all bytes in `out`, and since `T` implements + // `FromBytes`, any bit-pattern is a valid value for this type. + Ok(unsafe { out.assume_init() }) + } + /// Reads the entirety of the user slice, appending it to the end of the provided buffer. /// /// Fails with `EFAULT` if the read happens on a bad address. @@ -301,4 +338,34 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result { self.length -= len; Ok(()) } + + /// Writes the provided Rust value to this userspace pointer. + /// + /// Fails with `EFAULT` if the write encounters a page fault. + pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { + let len = size_of::<T>(); + if len > self.length { + return Err(EFAULT); + } + let Ok(len_ulong) = c_ulong::try_from(len) else { + return Err(EFAULT); + }; + // SAFETY: The reference points to a value of type `T`, so it is valid for reading + // `size_of::<T>()` bytes. + // + // By using the _copy_to_user variant, we skip the check_object_size check that verifies the + // kernel pointer. This mirrors the logic on the C side that skips the check when the length + // is a compile-time constant. + let res = unsafe { + bindings::_copy_to_user(self.ptr, (value as *const T).cast::<c_void>(), len_ulong) + }; + if res != 0 { + return Err(EFAULT); + } + // Since this is not a pointer to a valid object in our program, we cannot use `add`, which + // has C-style rules for defined behavior. + self.ptr = self.ptr.wrapping_byte_add(len); + self.length -= len; + Ok(()) + } }