Re: [PATCH 01/18] rust: add untrusted data abstraction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing owned interface causes memory leaks for `Untrusted<MaybeUninit<T>>`.
- [High] Calling `.len()` after `.as_mut_ptr()` on the same slice triggers Undefined Behavior under Stacked Borrows.
- [Medium] Unaligned reference creation in `Validate` documentation examples triggers Undefined Behavior.
- [Low] Missing `#[inline]` annotations on abstraction functions.
- [Low] Nested imports violate the vertical import style guidelines.
- [Low] Incomplete sentences, wrong examples, and inaccurate safety comments in documentation.
--

commit 2a47d2c142208915fc2ad9542e62ff816065513b
Author: Benno Lossin <lossin@kernel.org>

rust: add untrusted data abstraction

This commit introduces the Untrusted and Unvalidated abstractions for handling
data from userspace or hardware. It uses the Validate trait to enforce
validation before data can be safely accessed.

Message-ID: <20240925205244.873020-2-benno.lossin@proton.me>

> diff --git a/rust/kernel/validate.rs b/rust/kernel/validate.rs
> new file mode 100644
> index 0000000000000..ae0aa20e27b47
> --- /dev/null
> +++ b/rust/kernel/validate.rs
> @@ -0,0 +1,605 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +use crate::prelude::Init;
> +use core::{
> +    mem::MaybeUninit,
> +    ops::{Index, IndexMut},
> +    ptr, slice,
> +};

This isn't a bug, but should ptr and slice be on separate lines to follow the
vertical import style guidelines?

[ ... ]

> +impl<T: ?Sized> Untrusted<T> {
> +    /// Marks the given value as untrusted.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::validate::Untrusted;
> +    ///
> +    /// # mod bindings { pub(crate) unsafe fn read_foo_info() -> [u8; 4] { todo!() } };
> +    /// fn read_foo_info() -> Untrusted<[u8; 4]> {
> +    ///     // SAFETY: just an FFI call without preconditions.
> +    ///     Untrusted::new(unsafe { bindings::read_foo_info() })
> +    /// }
> +    /// ```
> +    pub fn new(value: T) -> Self
> +    where
> +        T: Sized,
> +    {
> +        Self(Unvalidated::new(value))
> +    }

Should small abstraction functions like new(), new_ref(), new_mut(), and
validate() be annotated with #[inline] per the Rust subsystem coding
guidelines?

[ ... ]

> +    ///         // SAFETY: `data` returned by `foo_hardware_read` is valid for reads as long as the
> +    ///         // `foo_hardware` object exists. That function updated the
> +    ///         let data = unsafe { &mut *data };
> +    ///         Ok(Untrusted::new_mut(data))
> +    ///     }
> +    /// }
> +    /// ```
> +    pub fn new_mut(value: &mut T) -> &mut Self {

This isn't a bug, but it looks like the comment sentence "That function updated
the" is incomplete.

[ ... ]

> +    /// 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>]>
> +    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()) };

This isn't a bug, but the safety comment mentions a "mutable slice reference",
while the input slice is an immutable reference. Could this be corrected?

> +        Untrusted::new_ref(slice)
> +    }
> +
> +    /// Turns a slice of uninitialized, untrusted values into an untrusted slice of uninitialized
> +    /// values.
> +    pub fn transpose_slice_uninit_mut(
> +        slice: &mut [MaybeUninit<Untrusted<T>>],
> +    ) -> &mut Untrusted<[MaybeUninit<T>]>
> +    where
> +        T: Sized,
> +    {
> +        // CAST: `MaybeUninit<T>` and `MaybeUninit<Untrusted<T>>` have the same layout.
> +        let ptr = slice.as_mut_ptr().cast::<MaybeUninit<T>>();
> +        // SAFETY: `ptr` and `len` come from the same mutable slice reference.
> +        let slice = unsafe { slice::from_raw_parts_mut(ptr, slice.len()) };

Could calling slice.len() after slice.as_mut_ptr() trigger undefined behavior
under Stacked Borrows?

Calling slice.len() creates a shared reference that invalidates the previously
derived mutable raw pointer.

Should the length be stored in a local variable before calling
slice.as_mut_ptr()?

> +        Untrusted::new_mut(slice)
> +    }
> +}
> +
> +impl<T> Untrusted<MaybeUninit<T>> {
> +    /// Sets the underlying untrusted value.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::validate::Untrusted;
> +    ///
> +    /// let mut untrusted = Untrusted::new(42);
> +    /// untrusted.write(24);
> +    /// ```
> +    pub fn write_uninit<E>(&mut self, value: impl Init<T, E>) -> Result<&mut Untrusted<T>, E> {

This isn't a bug, but the example initializes Untrusted::new(42) instead of
using MaybeUninit, and calls write() instead of write_uninit().

Additionally, does this interface leak owned resources?

There is no assume_init() or into_inner() method to consume the Untrusted
MaybeUninit type by value and return the initialized Untrusted type.

Because MaybeUninit does not drop the inner type when it goes out of scope,
it seems any initialized resource requiring a destructor will be permanently
leaked.

[ ... ]

> +impl<T> Untrusted<[MaybeUninit<T>]> {
> +    /// Sets the underlying untrusted value.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::validate::Untrusted;
> +    ///
> +    /// let mut untrusted = Untrusted::new(42);
> +    /// untrusted.write(24);
> +    /// ```
> +    pub fn write_uninit_slice<E>(
> +        &mut self,
> +        value: impl Init<[T], E>,
> +    ) -> Result<&mut Untrusted<[T]>, E> {
> +        let ptr: *mut [MaybeUninit<T>] = &mut self.0 .0;
> +        // CAST: `MaybeUninit<T>` is `repr(transparent)`.
> +        let ptr = ptr as *mut [T];
> +        // SAFETY: `ptr` came from a reference and if `Err` is returned, the underlying memory is
> +        // considered uninitialized.
> +        unsafe { value.__init(ptr) }.map(|()| {
> +            let this = self.0.raw_mut().as_mut_ptr();
> +            // CAST: `MaybeUninit<T>` is `repr(transparent)`.
> +            let this = this.cast::<T>();
> +            // SAFETY: `this` and `len` came from the same slice reference.
> +            let this = unsafe { slice::from_raw_parts_mut(this, self.0.len()) };

Similar to transpose_slice_uninit_mut(), does calling self.0.len()
invalidate the mutable pointer derived from self.0.raw_mut().as_mut_ptr()?

[ ... ]

> +///         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 })
> +///     }
> +/// }

Could dereferencing this pointer trigger undefined behavior due to unaligned
access?

A byte slice &[u8] only guarantees 1-byte alignment, so the resulting
pointer might not satisfy the 8-byte alignment required by the Data struct.

Should the example verify alignment before dereferencing?

[ ... ]

> +///         // 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();

This isn't a bug, but there appears to be an unused if block here.

[ ... ]

> +    fn new_ref(value: &T) -> &Self {
> +        let ptr: *const T = value;
> +        // CAST: `Self` is `repr(transparent)` and contains a `T`.
> +        let ptr = ptr as *const Self;
> +        // SAFETY: `ptr` came from a mutable reference valid for `'a`.
> +        unsafe { &*ptr }
> +    }

This isn't a bug, but the safety comment says the pointer came from a "mutable
reference", yet the value is an immutable reference. Could this be fixed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508031710.514574-1-alistair.francis@wdc.com?part=1