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

[Dirk Behme]

Hi Benno,

just some quick findings:


On 25.09.2024 22:53, Benno Lossin wrote:
....
> diff --git a/rust/kernel/validate.rs b/rust/kernel/validate.rs
> new file mode 100644
> index 000000000000..b325349e7dc3
> --- /dev/null
> +++ b/rust/kernel/validate.rs
> @@ -0,0 +1,604 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Types for handling and validating untrusted data.
> +//!
> +//! # Overview
> +//!
> +//! Untrusted data is marked using the [`Untrusted<T>`] type. See [Rationale](#rationale) for the
> +//! reasons to mark untrusted data throught the kernel.


Typo? throught -> through (i.e. drop the trailing 't')?


...
> //! [`Untrusted<T>`]. This type only allows validating the data or 
passing it along, since copying
> //! data from one userspace buffer into another is allowed for 
untrusted data.

I wonder if we should drop the "userspace"? I mean untrusted data must 
not be in a user space buffer, only? It could come e.g. from hardware as 
well. Like in the read_bytes_from_network() example.

...
> +    /// Marks the value behind the reference as untrusted.
> +    ///
> +    /// # Examples
> +    ///
> +    /// In this imaginary example there exists the `foo_hardware` struct on the C side, as well as
> +    /// a `foo_hardware_read` function that reads some data directly from the hardware.
> +    /// ```
> +    /// use kernel::{error, types::Opaque, validate::Untrusted};
> +    /// use core::ptr;
> +    ///
> +    /// # #[allow(non_camel_case_types)]
> +    /// # mod bindings {
> +    /// #     pub(crate) struct foo_hardware;
> +    /// #     pub(crate) unsafe fn foo_hardware_read(_foo: *mut foo_hardware, _len: &mut usize) -> *mut u8 {
> +    /// #         todo!()
> +    /// #     }
> +    /// # }
> +    /// struct Foo(Opaque<bindings::foo_hardware>);
> +    ///
> +    /// impl Foo {
> +    ///     fn read(&mut self, mut len: usize) -> Result<&Untrusted<[u8]>> {
> +    ///         // SAFETY: just an FFI call without preconditions.
> +    ///         let data: *mut u8 = unsafe { bindings::foo_hardware_read(self.0.get(), &mut len) };
> +    ///         let data = error::from_err_ptr(data)?;


With my heavily patched 6.11-rc1 base for this I get:

error[E0603]: function `from_err_ptr` is private
    --> rust/doctests_kernel_generated.rs:6749:27
     |
6749 |         let data = error::from_err_ptr(data)?;
     |                           ^^^^^^^^^^^^ private function
     |
note: the function `from_err_ptr` is defined here
    --> rust/kernel/error.rs:271:1
     |
271  | pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


And the same for the &mut Untrusted example.


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

For this I get

   --> rust/kernel/validate.rs:188:15
    |
188 |         match unsafe { value.__init(ptr) } {
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pattern `Err(_)` not 
covered
    |
note: `core::result::Result<(), Infallible>` defined here
   --> 
.rustup/toolchains/1.80.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:527:1
    |
527 | pub enum Result<T, E> {
    | ^^^^^^^^^^^^^^^^^^^^^
...
536 |     Err(#[stable(feature = "rust1", since = "1.0.0")] E),
    |     --- not covered


Just fyi in case its not due to my local patching ;)


Best regards

Dirk