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

From: Dirk Behme
Date: Thu Sep 26 2024 - 03:08:48 EST


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