Re: [PATCH v2 1/2] rust: add untrusted data abstraction

Benno Lossin
2024-09-30T14:04:18Z

On 26.09.24 12:40, Simona Vetter wrote:
> On Wed, Sep 25, 2024 at 08:53:05PM +0000, Benno Lossin wrote:
>> + /// Sets the underlying untrusted value.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::validate::Untrusted;
>> + ///
>> + /// let mut untrusted = Untrusted::new(42);
>> + /// untrusted.write(24);
>> + /// ```
>> + pub fn write(&mut self, value: impl Init<T>) {
>> + let ptr: *mut T = &mut self.0 .0;
>> + // SAFETY: `ptr` came from a mutable reference and the value is overwritten before it is
>> + // read.
>> + unsafe { ptr::drop_in_place(ptr) };
>> + // SAFETY: `ptr` came from a mutable reference and the initializer cannot error.
>> + match unsafe { value.__init(ptr) } {
>> + Ok(()) => {}
>> + }
>> + }
>
> So I think this doesn't work for a few reasons:
>
> - there's a lot of unsafe both here and when using this (even without
> MaybeUninit), at least if I understand things correctly. And forcing
> unsafe for something that inheritedly does not break any of the rust
> memory safety rules feels silly.
>
> - it also looks awkward, because you have the special versions
> write_uninit and write_uninit_slice, and imo if our write isn't good
> enought to just work with abritrary box/container types, it's not good.
>
> - it doesn't actually work for one of the main uaccess.rs use-cases beyond
> just enabling the current interface. Example in pseudo-rust:
>
> struct IoctlParams {
> input: u32,
> ouptut: u32,
> }
>
> The thing is that ioctl that use the struct approach like drm does, use
> the same struct if there's both input and output paramterers, and
> furthermore we are not allowed to overwrite the entire struct because
> that breaks ioctl restarting. So the flow is roughly
>
> let userptr : UserSlice;
> let params : Untrusted<IoctlParams>;
>
> userptr.read(params));
>
> // validate params, do something interesting with it params.input
>
> // this is _not_ allowed to overwrite params.input but must leave it
> // unchanged
>
> params.write(|x| { x.output = 42; });
>
> userptr.write(params);
>
> Your current write doesn't allow this case, and I think that's not good
> enough.

I see. One thing that makes this API work better is field projections
(it's a rust language feature for which we do have a PoC macro
implementation and which I am working on getting into the language):
To only overwrite a part of a struct you would do

params->output.write(42);

The `->` syntax "projects" the `output` field, turning
`Untrusted<IoctlParams>` into `Untrusted<u32>`.

I think that this would solve your problem with the API. (we probably
need a stop gap solution though until field projections are part of the
language).

> The one I propsed in private does:
>
> Untrusted<T>::write(&mut self, Fn(&mut T)->())
>
> It's not perfect because it allows you to do other stuff than writing,
> but it's still pretty good by limiting any potentially dangerous code to
> the provided closure. And with these of protection apis we need to
> strike the right balance between as much protection as possible while
> still allowing users to get the job done.

Ideally I would like to avoid exposing the value.

> Now I guess you can do this with your write too using a copy constructor
> approach, but that will result in less efficient code and it's probably
> also too much functional programming design for kernel developers to
> cope with.

Agreed.

>> +
>> + /// Turns a slice of untrusted values into an untrusted slice of values.
>> + pub fn transpose_slice(slice: &[Untrusted<T>]) -> &Untrusted<[T]>
>> + where
>> + T: Sized,
>> + {
>> + let ptr = slice.as_ptr().cast::<T>();
>> + // SAFETY: `ptr` and `len` come from the same slice reference.
>> + let slice = unsafe { slice::from_raw_parts(ptr, slice.len()) };
>> + Untrusted::new_ref(slice)
>> + }
>> +
>> + /// Turns a slice of uninitialized, untrusted values into an untrusted slice of uninitialized
>> + /// values.
>> + pub fn transpose_slice_uninit(
>> + slice: &[MaybeUninit<Untrusted<T>>],
>> + ) -> &Untrusted<[MaybeUninit<T>]>
>
> So these and some of the related functions to handle slice of box vs box
> of slice feel a bit awkward. I think we can do better if we rename
> Untrusted and Unvalidated to UntrustedBox and UnvalidatedBox, and then
> make Untrusted and Unvalidated traits, with implementations for
> UntrustedBox<T>, [Untrusted<T>], and all the others we want.

Hmm that might work for `Untrusted`, I will see how that looks. For
`Unvalidated` I am less confident.

> I expect that in the future we'll get more boxes with special semantics
> that we want to use together with Untrusted, not just [] and MaybeUninit.
> One example would be when the Untrusted data is shared with userspace
> (mmap()ed into both kernel and userspace for example for a ringbuffer), in
> which case the data is both Untrusted but also SharedUnsafe or whatever
> we'll call memory that's fundamentally breaking the rust memory model
> because there's no exclusive access (userspace can always do whatever it
> feels like), and all access has to go through at least
> READ_ONCE/WRITE_ONCE from the C side, or often even full blown atomics.

I don't think that we need to wrap that in `Untrusted`, instead the type
representing shared data between kernel and userspace should return
`Untrusted<u8>` instead of `u8`.

> And that memory is not MaybeUninit, because we have to initialize it
> before userspace can mmap it, for otherwise it's an information leak and
> hence security issue.
>
> tldr; I think going with traits for Untrusted and Unvalidated here will be
> worth it, even if a bit more pain at first. Plus cleaner interfaces.
>
>> + where
>> + T: Sized,
>> + {
>> + let ptr = slice.as_ptr().cast::<MaybeUninit<T>>();
>> + // SAFETY: `ptr` and `len` come from the same mutable slice reference.
>> + let slice = unsafe { slice::from_raw_parts(ptr, slice.len()) };
>> + Untrusted::new_ref(slice)
>> + }

[...]

>> +/// Validates untrusted data.
>> +///
>> +/// # Examples
>> +///
>> +/// The simplest way to validate data is to just implement `Validate<&Unvalidated<[u8]>>` for the
>> +/// type that you wish to validate:
>> +///
>> +/// ```
>> +/// use kernel::{
>> +/// error::{code::EINVAL, Error},
>> +/// str::{CStr, CString},
>> +/// validate::{Unvalidated, Validate},
>> +/// };
>> +///
>> +/// struct Data {
>> +/// flags: u8,
>> +/// name: CString,
>> +/// }
>> +///
>> +/// impl Validate<&Unvalidated<[u8]>> for Data {
>> +/// type Err = Error;
>> +///
>> +/// fn validate(unvalidated: &Unvalidated<[u8]>) -> Result<Self, Self::Err> {
>> +/// let raw = unvalidated.raw();
>> +/// let (&flags, name) = raw.split_first().ok_or(EINVAL)?;
>> +/// let name = CStr::from_bytes_with_nul(name)?.to_cstring()?;
>> +/// Ok(Data { flags, name })
>> +/// }
>> +/// }
>> +/// ```
>> +///
>> +/// This approach copies the data and requires allocation. If you want to avoid the allocation and
>> +/// copying the data, you can borrow from the input like this:
>> +///
>> +/// ```
>> +/// use kernel::{
>> +/// error::{code::EINVAL, Error},
>> +/// str::CStr,
>> +/// validate::{Unvalidated, Validate},
>> +/// };
>> +///
>> +/// struct Data<'a> {
>> +/// flags: u8,
>> +/// name: &'a CStr,
>> +/// }
>> +///
>> +/// impl<'a> Validate<&'a Unvalidated<[u8]>> for Data<'a> {
>> +/// type Err = Error;
>> +///
>> +/// fn validate(unvalidated: &'a Unvalidated<[u8]>) -> Result<Self, Self::Err> {
>> +/// let raw = unvalidated.raw();
>> +/// let (&flags, name) = raw.split_first().ok_or(EINVAL)?;
>> +/// let name = CStr::from_bytes_with_nul(name)?;
>> +/// Ok(Data { flags, name })
>> +/// }
>> +/// }
>> +/// ```
>> +///
>> +/// If you need to in-place validate your data, you currently need to resort to `unsafe`:
>> +///
>> +/// ```
>> +/// use kernel::{
>> +/// error::{code::EINVAL, Error},
>> +/// str::CStr,
>> +/// validate::{Unvalidated, Validate},
>> +/// };
>> +/// use core::mem;
>> +///
>> +/// // Important: use `repr(C)`, this ensures a linear layout of this type.
>> +/// #[repr(C)]
>> +/// struct Data {
>> +/// version: u8,
>> +/// flags: u8,
>> +/// _reserved: [u8; 2],
>> +/// count: u64,
>> +/// // lots of other fields...
>> +/// }
>> +///
>> +/// impl Validate<&Unvalidated<[u8]>> for &Data {
>> +/// type Err = Error;
>> +///
>> +/// fn validate(unvalidated: &Unvalidated<[u8]>) -> Result<Self, Self::Err> {
>> +/// let raw = unvalidated.raw();
>> +/// if raw.len() < mem::size_of::<Data>() {
>> +/// return Err(EINVAL);
>> +/// }
>> +/// // can only handle version 0
>> +/// if raw[0] != 0 {
>> +/// return Err(EINVAL);
>> +/// }
>> +/// // version 0 only uses the lower 4 bits of flags
>> +/// if raw[1] & 0xf0 != 0 {
>> +/// return Err(EINVAL);
>> +/// }
>> +/// let ptr = raw.as_ptr();
>> +/// // CAST: `Data` only contains integers and has `repr(C)`.
>> +/// let ptr = ptr.cast::<Data>();
>> +/// // SAFETY: `ptr` came from a reference and the cast above is valid.
>> +/// Ok(unsafe { &*ptr })
>> +/// }
>> +/// }
>> +/// ```
>> +///
>> +/// To be able to modify the parsed data, while still supporting zero-copy, you can implement
>> +/// `Validate<&mut Unvalidated<[u8]>>`:
>> +///
>> +/// ```
>> +/// use kernel::{
>> +/// error::{code::EINVAL, Error},
>> +/// str::CStr,
>> +/// validate::{Unvalidated, Validate},
>> +/// };
>> +/// use core::mem;
>> +///
>> +/// // Important: use `repr(C)`, this ensures a linear layout of this type.
>> +/// #[repr(C)]
>> +/// struct Data {
>> +/// version: u8,
>> +/// flags: u8,
>> +/// _reserved: [u8; 2],
>> +/// count: u64,
>> +/// // lots of other fields...
>> +/// }
>> +///
>> +/// impl Validate<&mut Unvalidated<[u8]>> for &Data {
>> +/// type Err = Error;
>
> I think we should make that one the default, but not sure that's doable
> with associated types instead of generics.

There is the `associated_type_defaults` unstable feature:
https://github.com/rust-lang/rust/issues/29661

But I actually think that we should get away from making the `Error`
type the return error of Rust functions. It's much better to have
function-specific errors, since they can be more descriptive. In cases
where we have to return to userspace or C, sure use the existing
`Error`.

> The input parameter should definitely default to the output paramter,
> because often they're just exactly the same e.g. when validating ioctl
> input structures.

I don't understand what you mean by this? The input parameter has to be
some sort of `..Untrusted..` and the output shouldn't have that.

> On this version you've also left out the in-place validation (originally
> validate_bytes), but we can add that later on when a need arises I guess.
> Or do you think it's just not a good idea in general?

This version supports in-place validation, this example and the one
above showcase how you would implement `Validate` in that case.

You can then call `.vaildate()` on an `&[mut ]Unvalidated<[u8]>` and you
get a `&Data` that borrows from the unvalidated data without copying the
data.

---
Cheers,
Benno

>> +///
>> +/// fn validate(unvalidated: &mut Unvalidated<[u8]>) -> Result<Self, Self::Err> {
>> +/// let raw = unvalidated.raw_mut();
>> +/// if raw.len() < mem::size_of::<Data>() {
>> +/// return Err(EINVAL);
>> +/// }
>> +/// match raw[0] {
>> +/// 0 => {},
>> +/// 1 => {
>> +/// // version 1 implicitly sets the first bit.
>> +/// raw[1] |= 1;
>> +/// },
>> +/// // can only handle version 0 and 1
>> +/// _ => return Err(EINVAL),
>> +/// }
>> +/// // version 0 and 1 only use the lower 4 bits of flags
>> +/// if raw[1] & 0xf0 != 0 {
>> +/// return Err(EINVAL);
>> +/// }
>> +/// if raw[1] == 0 {}
>> +/// let ptr = raw.as_ptr();
>> +/// // CAST: `Data` only contains integers and has `repr(C)`.
>> +/// let ptr = ptr.cast::<Data>();
>> +/// // SAFETY: `ptr` came from a reference and the cast above is valid.
>> +/// Ok(unsafe { &*ptr })
>> +/// }
>> +/// }
>> +/// ```
>> +pub trait Validate<I: ValidateInput>: Sized {
>> + /// Validation error.
>> + type Err;
>> +
>> + /// Validate the given untrusted data and parse it into the output type.
>> + fn validate(unvalidated: I) -> Result<Self, Self::Err>;
>> +}