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

Dirk Behme
2024-09-26T07:08:37Z

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