Message-ID: <80b7923e-78a0-4de6-bf95-ad7b45171a40@de.bosch.com>
Date: Fri, 29 Aug 2025 07:23:08 +0200
From: Dirk Behme <dirk.behme@...bosch.com>
To: Benno Lossin <lossin@...nel.org>, Simona Vetter <simona.vetter@...ll.ch>,
	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>, Danilo Krummrich <dakr@...nel.org>, Wedson Almeida Filho
	<wedsonaf@...il.com>, Viresh Kumar <viresh.kumar@...aro.org>, "Tamir
 Duberstein" <tamird@...il.com>, Xiangfei Ding <dingxiangfei2009@...il.com>,
	Greg KH <gregkh@...uxfoundation.org>
CC: Benno Lossin <benno.lossin@...ton.me>, <linux-kernel@...r.kernel.org>,
	<rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] rust: create basic untrusted data API

Hi Benno,

two minor comments:

On 14/08/2025 14:44, Benno Lossin wrote:
> From: Benno Lossin <benno.lossin@...ton.me>
> 
> When the kernel receives external data (e.g. from userspace), it usually
> is a very bad idea to directly use the data for logic decision in the
> kernel. For this reason, such data should be explicitly marked and
> validated before making decision based on its value.
> 
> The `Untrusted<T>` wrapper type marks a value of type `T` as untrusted.
> The particular meaning of "untrusted" highly depends on the type `T`.
> For example `T = u8` ensures that the value of the byte cannot be
> retrieved. However, `T = [u8]` still allows to access the length of the
> slice. Similarly, `T = KVec<U>` allows modifications.
> 
> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
> ---
>  rust/kernel/lib.rs      |   1 +
>  rust/kernel/validate.rs | 142 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
>  create mode 100644 rust/kernel/validate.rs
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 99dbb7b2812e..f9dcf079b903 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -128,6 +128,7 @@
>  pub mod transmute;
>  pub mod types;
>  pub mod uaccess;
> +pub mod validate;
>  pub mod workqueue;
>  pub mod xarray;
>  
> diff --git a/rust/kernel/validate.rs b/rust/kernel/validate.rs
> new file mode 100644
> index 000000000000..1f75ccb79532
> --- /dev/null
> +++ b/rust/kernel/validate.rs
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Untrusted data API.
> +//!
> +//! # Overview
> +//!
> +//! Untrusted data is marked using the [`Untrusted<T>`] type. See [Rationale](#rationale) for the
> +//! reasons to mark untrusted data throughout the kernel. It is a totally opaque wrapper, it is not
> +//! possible to read the data inside.
> +//!
> +//! APIs that write back into userspace usually allow writing untrusted bytes directly, allowing
> +//! direct copying of untrusted user data back into userspace without validation.
> +//!
> +//! # Rationale
> +//!
> +//! When reading data from an untrusted source, it must be validated before it can be used for
> +//! **logic**. For example, this is a very bad idea:
> +//!
> +//! ```
> +//! # fn read_bytes_from_network() -> KBox<[u8]> {
> +//! #     Box::new([1, 0], kernel::alloc::flags::GFP_KERNEL).unwrap()
> +//! # }
> +//! let bytes: KBox<[u8]> = read_bytes_from_network();
> +//! let data_index = bytes[0];
> +//! let data = bytes[usize::from(data_index)];
> +//! ```
> +//!
> +//! While this will not lead to a memory violation (because the array index checks the bounds), it
> +//! might result in a kernel panic. For this reason, all untrusted data must be wrapped in
> +//! [`Untrusted<T>`]. This type only allows validating the data or passing it along, since copying
> +//! data from userspace back into userspace is allowed for untrusted data.
> +
> +use core::ops::{Deref, DerefMut};
> +
> +use crate::{
> +    alloc::{Allocator, Vec},
> +    transmute::{cast_slice, cast_slice_mut},
> +};
> +
> +/// Untrusted data of type `T`.
> +///
> +/// Data coming from userspace is considered untrusted and should be marked by this type.


Regarding "data coming from userspace": It's not limited to userspace,
no? In the example above you use "read from network". I like the wording
from the commit message more: "external data (e.g. from userspace)". Or
"untrusted source".


> +///
> +/// The particular meaning of [`Untrusted<T>`] depends heavily on the type `T`. For example,
> +/// `&Untrusted<[u8]>` is a reference to an untrusted slice. But the length is not considered
> +/// untrusted, as it would otherwise violate normal Rust rules. For this reason, one can easily
> +/// convert that reference to `&[Untrusted<u8>]`. Another such example is `Untrusted<KVec<T>>`, it
> +/// derefs to `KVec<Untrusted<T>>`. Raw bytes however do not behave in this way, `Untrusted<u8>` is
> +/// totally opaque.
> +///
> +/// # Usage in API Design
> +///
> +/// The exact location where to put [`Untrusted`] depends on the kind of API. When asking for an
> +/// untrusted input value, or buffer to write to, always move the [`Untrusted`] wrapper as far
> +/// inwards as possible:
> +///
> +/// ```ignore
> +/// // use this
> +/// pub fn read_from_userspace(buf: &mut [Untrusted<u8>]) { todo!() }
> +///
> +/// // and not this
> +/// pub fn read_from_userspace(buf: &mut Untrusted<[u8]>) { todo!() }
> +/// ```
> +///
> +/// The reason for this is that `&mut Untrusted<[u8]>` can beconverted into `&mut [Untrusted<u8>]`


Missing space? "be converted"

Thanks!

Dirk

Powered by blists - more mailing lists