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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d4520031-2f3e-43a1-9e95-e7845ef8cb94@de.bosch.com>
Date: Thu, 26 Sep 2024 09:08:09 +0200
From: Dirk Behme <dirk.behme@...bosch.com>
To: Benno Lossin <benno.lossin@...ton.me>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, "Gary
 Guo" <gary@...yguo.net>, Björn Roy Baron
	<bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>
CC: Greg KH <gregkh@...uxfoundation.org>, Simona Vetter
	<simona.vetter@...ll.ch>, <linux-kernel@...r.kernel.org>,
	<rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] rust: add untrusted data abstraction

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