* [PATCH v4 1/4] rust: transmute: add `cast_slice[_mut]` functions
2025-08-14 12:44 [PATCH v4 0/4] Untrusted Data API Benno Lossin
@ 2025-08-14 12:44 ` Benno Lossin
2025-08-14 12:44 ` [PATCH v4 2/4] rust: create basic untrusted data API Benno Lossin
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Benno Lossin @ 2025-08-14 12:44 UTC (permalink / raw)
To: Greg KH, Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Fiona Behrens,
Aliet Exposito Garcia
Cc: Benno Lossin, rust-for-linux, linux-kernel
From: Benno Lossin <benno.lossin@proton.me>
Add functions to make casting slices only one `unsafe` block.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/transmute.rs | 60 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
index 1c7d43771a37..92410a93882f 100644
--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -2,6 +2,8 @@
//! Traits for transmuting types.
+use core::slice;
+
/// Types for which any bit pattern is valid.
///
/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
@@ -69,3 +71,61 @@ macro_rules! impl_asbytes {
{<T: AsBytes>} [T],
{<T: AsBytes, const N: usize>} [T; N],
}
+
+/// Casts the type of a slice to another.
+///
+/// Also see [`cast_slice_mut`].
+///
+/// # Examples
+///
+/// ```rust
+/// # use kernel::transmute::cast_slice;
+/// #[repr(transparent)]
+/// #[derive(Debug)]
+/// struct Container<T>(T);
+///
+/// let array = [0u32; 42];
+/// let slice = &array;
+/// // SAFETY: `Container<u32>` transparently wraps a `u32`.
+/// let container_slice = unsafe { cast_slice::<u32, Container<u32>>(slice) };
+/// pr_info!("{container_slice:?}");
+/// ```
+///
+/// # Safety
+///
+/// - `T` and `U` must have the same layout.
+pub unsafe fn cast_slice<T, U>(slice: &[T]) -> &[U] {
+ // CAST: by the safety requirements, `T` and `U` have the same layout.
+ let ptr = slice.as_ptr().cast::<U>();
+ // SAFETY: `ptr` and `len` come from the same slice reference.
+ unsafe { slice::from_raw_parts(ptr, slice.len()) }
+}
+
+/// Casts the type of a slice to another.
+///
+/// Also see [`cast_slice`].
+///
+/// # Examples
+///
+/// ```rust
+/// # use kernel::transmute::cast_slice_mut;
+/// #[repr(transparent)]
+/// #[derive(Debug)]
+/// struct Container<T>(T);
+///
+/// let mut array = [0u32; 42];
+/// let slice = &mut array;
+/// // SAFETY: `Container<u32>` transparently wraps a `u32`.
+/// let container_slice = unsafe { cast_slice_mut::<u32, Container<u32>>(slice) };
+/// pr_info!("{container_slice:?}");
+/// ```
+///
+/// # Safety
+///
+/// - `T` and `U` must have the same layout.
+pub unsafe fn cast_slice_mut<T, U>(slice: &mut [T]) -> &mut [U] {
+ // CAST: by the safety requirements, `T` and `U` have the same layout.
+ let ptr = slice.as_mut_ptr().cast::<U>();
+ // SAFETY: `ptr` and `len` come from the same slice reference.
+ unsafe { slice::from_raw_parts_mut(ptr, slice.len()) }
+}
--
2.50.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 2/4] rust: create basic untrusted data API
2025-08-14 12:44 [PATCH v4 0/4] Untrusted Data API Benno Lossin
2025-08-14 12:44 ` [PATCH v4 1/4] rust: transmute: add `cast_slice[_mut]` functions Benno Lossin
@ 2025-08-14 12:44 ` Benno Lossin
2025-08-29 5:23 ` Dirk Behme
2025-08-14 12:44 ` [RFC PATCH v4 3/4] rust: validate: add `Validate` trait Benno Lossin
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Benno Lossin @ 2025-08-14 12:44 UTC (permalink / raw)
To: Greg KH, Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Wedson Almeida Filho,
Viresh Kumar, Tamir Duberstein, Xiangfei Ding
Cc: Benno Lossin, linux-kernel, rust-for-linux
From: Benno Lossin <benno.lossin@proton.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@proton.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.
+///
+/// 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>]`
+/// very easily, but the converse is not possible.
+///
+/// For the same reason, when returning untrusted data by-value, one should move the [`Untrusted`]
+/// wrapper as far outward as possible:
+///
+/// ```ignore
+/// // use this
+/// pub fn read_all_from_userspace() -> Untrusted<KVec<u8>> { todo!() }
+///
+/// // and not this
+/// pub fn read_all_from_userspace() -> KVec<Untrusted<u8>> { todo!() }
+/// ```
+///
+/// Here too the reason is that `KVec<Untrusted<u8>>` is more restrictive compared to
+/// `Untrusted<KVec<u8>>`.
+#[repr(transparent)]
+pub struct Untrusted<T: ?Sized>(T);
+
+impl<T: ?Sized> Untrusted<T> {
+ /// Marks the given value as untrusted.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::validate::Untrusted;
+ ///
+ /// # mod bindings { pub(crate) unsafe fn read_foo_info() -> [u8; 4] { todo!() } };
+ /// fn read_foo_info() -> Untrusted<[u8; 4]> {
+ /// // SAFETY: just an FFI call without preconditions.
+ /// Untrusted::new(unsafe { bindings::read_foo_info() })
+ /// }
+ /// ```
+ pub fn new(value: T) -> Self
+ where
+ T: Sized,
+ {
+ Self(value)
+ }
+}
+
+impl<T> Deref for Untrusted<[T]> {
+ type Target = [Untrusted<T>];
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: `Untrusted<T>` transparently wraps `T`.
+ unsafe { cast_slice(&self.0) }
+ }
+}
+
+impl<T> DerefMut for Untrusted<[T]> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ // SAFETY: `Untrusted<T>` transparently wraps `T`.
+ unsafe { cast_slice_mut(&mut self.0) }
+ }
+}
+
+impl<T, A: Allocator> Deref for Untrusted<Vec<T, A>> {
+ type Target = Vec<Untrusted<T>, A>;
+
+ fn deref(&self) -> &Self::Target {
+ let ptr: *const Untrusted<Vec<T, A>> = self;
+ // CAST: `Untrusted<T>` transparently wraps `T`.
+ let ptr: *const Vec<Untrusted<T>, A> = ptr.cast();
+ // SAFETY: `ptr` is derived from the reference `self`.
+ unsafe { &*ptr }
+ }
+}
+
+impl<T, A: Allocator> DerefMut for Untrusted<Vec<T, A>> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ let ptr: *mut Untrusted<Vec<T, A>> = self;
+ // CAST: `Untrusted<T>` transparently wraps `T`.
+ let ptr: *mut Vec<Untrusted<T>, A> = ptr.cast();
+ // SAFETY: `ptr` is derived from the reference `self`.
+ unsafe { &mut *ptr }
+ }
+}
--
2.50.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 2/4] rust: create basic untrusted data API
2025-08-14 12:44 ` [PATCH v4 2/4] rust: create basic untrusted data API Benno Lossin
@ 2025-08-29 5:23 ` Dirk Behme
0 siblings, 0 replies; 16+ messages in thread
From: Dirk Behme @ 2025-08-29 5:23 UTC (permalink / raw)
To: Benno Lossin, Simona Vetter, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Wedson Almeida Filho,
Viresh Kumar, Tamir Duberstein, Xiangfei Ding, Greg KH
Cc: Benno Lossin, linux-kernel, rust-for-linux
Hi Benno,
two minor comments:
On 14/08/2025 14:44, Benno Lossin wrote:
> From: Benno Lossin <benno.lossin@proton.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@proton.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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v4 3/4] rust: validate: add `Validate` trait
2025-08-14 12:44 [PATCH v4 0/4] Untrusted Data API Benno Lossin
2025-08-14 12:44 ` [PATCH v4 1/4] rust: transmute: add `cast_slice[_mut]` functions Benno Lossin
2025-08-14 12:44 ` [PATCH v4 2/4] rust: create basic untrusted data API Benno Lossin
@ 2025-08-14 12:44 ` Benno Lossin
2025-09-04 6:48 ` Dirk Behme
2025-08-14 12:44 ` [RFC PATCH v4 4/4] rust: iov: use untrusted data API Benno Lossin
2025-08-14 14:37 ` [PATCH v4 0/4] Untrusted Data API Greg KH
4 siblings, 1 reply; 16+ messages in thread
From: Benno Lossin @ 2025-08-14 12:44 UTC (permalink / raw)
To: Greg KH, Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich
Cc: Benno Lossin, rust-for-linux, linux-kernel
From: Benno Lossin <benno.lossin@proton.me>
Introduce the `Validate<Input>` trait and functions to validate
`Untrusted<T>` using said trait. This allows one to access the inner
value of `Untrusted<T>` via `validate{,_ref,_mut}` functions which
subsequently delegate the validation to user-implemented `Validate`
trait.
The `Validate` trait is the only entry point for validation code, making
it easy to spot where data is being validated.
The reason for restricting the types that can be inputs to
`Validate::validate` is to be able to have the `validate...` functions
on `Untrusted`. This is also the reason for the suggestions in the
`Usage in API Design` section in the commit that introduced
`Untrusted<T>`.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/validate.rs | 70 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 69 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/validate.rs b/rust/kernel/validate.rs
index 1f75ccb79532..2a582a572aa5 100644
--- a/rust/kernel/validate.rs
+++ b/rust/kernel/validate.rs
@@ -11,6 +11,9 @@
//! APIs that write back into userspace usually allow writing untrusted bytes directly, allowing
//! direct copying of untrusted user data back into userspace without validation.
//!
+//! The only way to access untrusted data is to [`Validate::validate`] it. This is facilitated by
+//! the [`Validate`] trait.
+//!
//! # Rationale
//!
//! When reading data from an untrusted source, it must be validated before it can be used for
@@ -46,7 +49,7 @@
/// 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.
+/// totally opaque and one can only access its value by calling [`Untrusted::validate()`].
///
/// # Usage in API Design
///
@@ -101,6 +104,30 @@ pub fn new(value: T) -> Self
{
Self(value)
}
+
+ /// Validate the underlying untrusted data.
+ ///
+ /// See the [`Validate`] trait for more information.
+ pub fn validate<V: Validate<Self>>(self) -> Result<V, V::Err>
+ where
+ T: Sized,
+ {
+ V::validate(self.0)
+ }
+
+ /// Validate the underlying untrusted data.
+ ///
+ /// See the [`Validate`] trait for more information.
+ pub fn validate_ref<'a, V: Validate<&'a Self>>(&'a self) -> Result<V, V::Err> {
+ V::validate(&self.0)
+ }
+
+ /// Validate the underlying untrusted data.
+ ///
+ /// See the [`Validate`] trait for more information.
+ pub fn validate_mut<'a, V: Validate<&'a mut Self>>(&'a mut self) -> Result<V, V::Err> {
+ V::validate(&mut self.0)
+ }
}
impl<T> Deref for Untrusted<[T]> {
@@ -140,3 +167,44 @@ fn deref_mut(&mut self) -> &mut Self::Target {
unsafe { &mut *ptr }
}
}
+
+/// Marks valid input for the [`Validate`] trait.
+pub trait ValidateInput: private::Sealed {
+ /// Type of the inner data.
+ type Inner: ?Sized;
+}
+
+impl<T: ?Sized> ValidateInput for Untrusted<T> {
+ type Inner = T;
+}
+
+impl<'a, T: ?Sized> ValidateInput for &'a Untrusted<T> {
+ type Inner = &'a T;
+}
+
+impl<'a, T: ?Sized> ValidateInput for &'a mut Untrusted<T> {
+ type Inner = &'a mut T;
+}
+
+mod private {
+ use super::Untrusted;
+
+ pub trait Sealed {}
+
+ impl<T: ?Sized> Sealed for Untrusted<T> {}
+ impl<'a, T: ?Sized> Sealed for &'a Untrusted<T> {}
+ impl<'a, T: ?Sized> Sealed for &'a mut Untrusted<T> {}
+}
+
+/// Validate [`Untrusted`] data.
+///
+/// Care must be taken when implementing this trait, as unprotected access to unvalidated data is
+/// given to the [`Validate::validate`] function. The implementer must ensure that the data is only
+/// used for logic after successful validation.
+pub trait Validate<Input: ValidateInput>: Sized {
+ /// Validation error.
+ type Err;
+
+ /// Validate the raw input.
+ fn validate(raw: Input::Inner) -> Result<Self, Self::Err>;
+}
--
2.50.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [RFC PATCH v4 3/4] rust: validate: add `Validate` trait
2025-08-14 12:44 ` [RFC PATCH v4 3/4] rust: validate: add `Validate` trait Benno Lossin
@ 2025-09-04 6:48 ` Dirk Behme
0 siblings, 0 replies; 16+ messages in thread
From: Dirk Behme @ 2025-09-04 6:48 UTC (permalink / raw)
To: Benno Lossin, Greg KH, Simona Vetter, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich
Cc: Benno Lossin, rust-for-linux, linux-kernel
On 14/08/2025 14:44, Benno Lossin wrote:
> From: Benno Lossin <benno.lossin@proton.me>
>
> Introduce the `Validate<Input>` trait and functions to validate
> `Untrusted<T>` using said trait. This allows one to access the inner
> value of `Untrusted<T>` via `validate{,_ref,_mut}` functions which
> subsequently delegate the validation to user-implemented `Validate`
> trait.
>
> The `Validate` trait is the only entry point for validation code, making
> it easy to spot where data is being validated.
>
> The reason for restricting the types that can be inputs to
> `Validate::validate` is to be able to have the `validate...` functions
> on `Untrusted`. This is also the reason for the suggestions in the
> `Usage in API Design` section in the commit that introduced
> `Untrusted<T>`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
While experimenting with this I was looking for an example how to use
`Validate::validate`. For example, illustrating how the "very bad idea"
read_bytes_from_network() example from patch 2/4 could be done correctly
with these patches would be nice.
Just to illustrate what I'm thinking about see [1].
Thanks!
Dirk
[1]
diff --git a/rust/kernel/validate.rs b/rust/kernel/validate.rs
index 2a582a572aa5e..688ccd541712a 100644
--- a/rust/kernel/validate.rs
+++ b/rust/kernel/validate.rs
@@ -82,6 +82,7 @@
/// Here too the reason is that `KVec<Untrusted<u8>>` is more
restrictive compared to
/// `Untrusted<KVec<u8>>`.
#[repr(transparent)]
+#[derive(Copy, Clone)]
pub struct Untrusted<T: ?Sized>(T);
impl<T: ?Sized> Untrusted<T> {
@@ -201,6 +202,49 @@ impl<'a, T: ?Sized> Sealed for &'a mut Untrusted<T> {}
/// Care must be taken when implementing this trait, as unprotected
access to unvalidated data is
/// given to the [`Validate::validate`] function. The implementer must
ensure that the data is only
/// used for logic after successful validation.
+///
+/// # Examples
+///
+///```
+/// # use kernel::validate::{Untrusted, Validate};
+///
+/// struct TrustedIndex(u8);
+/// struct TrustedData(u8);
+///
+/// # fn read_bytes_from_network() -> KBox<Untrusted<[u8]>> {
+/// # Box::new(Untrusted::new([1, 0]),
kernel::alloc::flags::GFP_KERNEL).unwrap()
+/// # }
+/// impl Validate<Untrusted<u8>> for TrustedIndex {
+/// type Err = Error;
+///
+/// fn validate(raw: u8) -> Result<Self, Self::Err> {
+/// if raw != 1 {
+/// pr_err!("Invalid index: {}\n", raw);
+/// return Err(EINVAL);
+/// }
+/// Ok(TrustedIndex(raw))
+/// }
+/// }
+///
+/// impl Validate<Untrusted<u8>> for TrustedData {
+/// type Err = Error;
+///
+/// fn validate(raw: u8) -> Result<Self, Self::Err> {
+/// // All raw data values are valid
+/// Ok(TrustedData(raw))
+/// }
+/// }
+///
+/// fn example() -> Result{
+/// let rawbytes = read_bytes_from_network();
+/// let index = rawbytes[0].validate::<TrustedIndex>()?;
+/// let data =
rawbytes[usize::from(index.0)].validate::<TrustedData>()?;
+/// assert_eq!(data.0, 0);
+/// Ok(())
+/// }
+/// # example()?;
+/// # Ok::<(), Error>(())
+/// ```
pub trait Validate<Input: ValidateInput>: Sized {
/// Validation error.
type Err;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH v4 4/4] rust: iov: use untrusted data API
2025-08-14 12:44 [PATCH v4 0/4] Untrusted Data API Benno Lossin
` (2 preceding siblings ...)
2025-08-14 12:44 ` [RFC PATCH v4 3/4] rust: validate: add `Validate` trait Benno Lossin
@ 2025-08-14 12:44 ` Benno Lossin
2025-08-14 14:37 ` [PATCH v4 0/4] Untrusted Data API Greg KH
4 siblings, 0 replies; 16+ messages in thread
From: Benno Lossin @ 2025-08-14 12:44 UTC (permalink / raw)
To: Greg KH, Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Arnd Bergmann
Cc: Benno Lossin, rust-for-linux, linux-kernel
From: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/iov.rs | 30 +++++++++++++++++++-----------
samples/rust/rust_misc_device.rs | 5 +++--
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/rust/kernel/iov.rs b/rust/kernel/iov.rs
index f55f8997ac2f..a53db5478e93 100644
--- a/rust/kernel/iov.rs
+++ b/rust/kernel/iov.rs
@@ -11,9 +11,11 @@
alloc::{Allocator, Flags},
bindings,
prelude::*,
+ transmute::cast_slice_mut,
types::Opaque,
+ validate::Untrusted,
};
-use core::{marker::PhantomData, mem::MaybeUninit, ptr, slice};
+use core::{marker::PhantomData, mem::MaybeUninit, slice};
const ITER_SOURCE: bool = bindings::ITER_SOURCE != 0;
const ITER_DEST: bool = bindings::ITER_DEST != 0;
@@ -126,11 +128,10 @@ pub unsafe fn revert(&mut self, bytes: usize) {
///
/// Returns the number of bytes that have been copied.
#[inline]
- pub fn copy_from_iter(&mut self, out: &mut [u8]) -> usize {
- // SAFETY: `Self::copy_from_iter_raw` guarantees that it will not write any uninitialized
- // bytes in the provided buffer, so `out` is still a valid `u8` slice after this call.
- let out = unsafe { &mut *(ptr::from_mut(out) as *mut [MaybeUninit<u8>]) };
-
+ pub fn copy_from_iter(&mut self, out: &mut [Untrusted<u8>]) -> usize {
+ // CAST: The call to `copy_from_iter_raw` below only writes initialized values.
+ // SAFETY: `Untrusted<T>` and `MaybeUninit<T>` transparently wrap a `T`.
+ let out: &mut [MaybeUninit<Untrusted<u8>>] = unsafe { cast_slice_mut(out) };
self.copy_from_iter_raw(out).len()
}
@@ -140,7 +141,7 @@ pub fn copy_from_iter(&mut self, out: &mut [u8]) -> usize {
#[inline]
pub fn copy_from_iter_vec<A: Allocator>(
&mut self,
- out: &mut Vec<u8, A>,
+ out: &mut Vec<Untrusted<u8>, A>,
flags: Flags,
) -> Result<usize> {
out.reserve(self.len(), flags)?;
@@ -158,7 +159,10 @@ pub fn copy_from_iter_vec<A: Allocator>(
///
/// This will never write uninitialized bytes to the provided buffer.
#[inline]
- pub fn copy_from_iter_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> &mut [u8] {
+ pub fn copy_from_iter_raw(
+ &mut self,
+ out: &mut [MaybeUninit<Untrusted<u8>>],
+ ) -> &mut [Untrusted<u8>] {
let capacity = out.len();
let out = out.as_mut_ptr().cast::<u8>();
@@ -172,7 +176,7 @@ pub fn copy_from_iter_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> &mut [u8] {
// SAFETY: The underlying C api guarantees that initialized bytes have been written to the
// first `len` bytes of the spare capacity.
- unsafe { slice::from_raw_parts_mut(out, len) }
+ unsafe { slice::from_raw_parts_mut(out.cast(), len) }
}
}
@@ -275,7 +279,7 @@ pub unsafe fn revert(&mut self, bytes: usize) {
/// Returns the number of bytes that were written. If this is shorter than the provided slice,
/// then no more bytes can be written.
#[inline]
- pub fn copy_to_iter(&mut self, input: &[u8]) -> usize {
+ pub fn copy_to_iter(&mut self, input: &[Untrusted<u8>]) -> usize {
// SAFETY:
// * By the struct invariants, it is still valid to write to this IO vector.
// * `input` is valid for `input.len()` bytes.
@@ -289,7 +293,11 @@ pub fn copy_to_iter(&mut self, input: &[u8]) -> usize {
/// that the file will appear to contain `contents` even if takes multiple reads to read the
/// entire file.
#[inline]
- pub fn simple_read_from_buffer(&mut self, ppos: &mut i64, contents: &[u8]) -> Result<usize> {
+ pub fn simple_read_from_buffer(
+ &mut self,
+ ppos: &mut i64,
+ contents: &[Untrusted<u8>],
+ ) -> Result<usize> {
if *ppos < 0 {
return Err(EINVAL);
}
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 9e4005e33796..6361c94da9a4 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -109,6 +109,7 @@
sync::Mutex,
types::ARef,
uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
+ validate::Untrusted,
};
const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
@@ -145,7 +146,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
struct Inner {
value: i32,
- buffer: KVVec<u8>,
+ buffer: Untrusted<KVec<u8>>,
}
#[pin_data(PinnedDrop)]
@@ -169,7 +170,7 @@ fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Se
RustMiscDevice {
inner <- new_mutex!(Inner {
value: 0_i32,
- buffer: KVVec::new(),
+ buffer: Untrusted::new(KVec::new()),
}),
dev: dev,
}
--
2.50.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-14 12:44 [PATCH v4 0/4] Untrusted Data API Benno Lossin
` (3 preceding siblings ...)
2025-08-14 12:44 ` [RFC PATCH v4 4/4] rust: iov: use untrusted data API Benno Lossin
@ 2025-08-14 14:37 ` Greg KH
2025-08-14 15:22 ` Benno Lossin
4 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2025-08-14 14:37 UTC (permalink / raw)
To: Benno Lossin
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
> I didn't have too much time to spend on this API, so this is mostly a
> resend of v3. There are some changes in the last commit, updating to the
> latest version of Alice's iov_iter patche series [1] & rebasing on top
> of v6.17-rc1.
>
> I think we should just merge the first two patches this cycle in order
> to get the initial, bare-bones API into the kernel and have people
> experiment with it. The validation logic in the third patch still needs
> some work and I'd need to find some time to work on that (no idea when I
> find it though).
Nice, thanks for reviving this!
And we should at least add an example using it, otherwise it's not going
to help out much here. Add it to the misc device driver api?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-14 14:37 ` [PATCH v4 0/4] Untrusted Data API Greg KH
@ 2025-08-14 15:22 ` Benno Lossin
2025-08-14 15:42 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Benno Lossin @ 2025-08-14 15:22 UTC (permalink / raw)
To: Greg KH
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
> On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
>> I didn't have too much time to spend on this API, so this is mostly a
>> resend of v3. There are some changes in the last commit, updating to the
>> latest version of Alice's iov_iter patche series [1] & rebasing on top
>> of v6.17-rc1.
>>
>> I think we should just merge the first two patches this cycle in order
>> to get the initial, bare-bones API into the kernel and have people
>> experiment with it. The validation logic in the third patch still needs
>> some work and I'd need to find some time to work on that (no idea when I
>> find it though).
>
> Nice, thanks for reviving this!
>
> And we should at least add an example using it, otherwise it's not going
> to help out much here. Add it to the misc device driver api?
You mean `rust/kernel/miscdevice.rs`? What parts of that API are
untrusted?
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-14 15:22 ` Benno Lossin
@ 2025-08-14 15:42 ` Greg KH
2025-08-14 17:23 ` Benno Lossin
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2025-08-14 15:42 UTC (permalink / raw)
To: Benno Lossin
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
> >> I didn't have too much time to spend on this API, so this is mostly a
> >> resend of v3. There are some changes in the last commit, updating to the
> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
> >> of v6.17-rc1.
> >>
> >> I think we should just merge the first two patches this cycle in order
> >> to get the initial, bare-bones API into the kernel and have people
> >> experiment with it. The validation logic in the third patch still needs
> >> some work and I'd need to find some time to work on that (no idea when I
> >> find it though).
> >
> > Nice, thanks for reviving this!
> >
> > And we should at least add an example using it, otherwise it's not going
> > to help out much here. Add it to the misc device driver api?
>
> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
> untrusted?
mmap() is, but you can't do anything about that...
ioctl() is the callback that is taking untrusted data from userspace.
That's one place we have had more kernel buffer overflows then I can
count and ALWAYS needs to be properly verified before anything can be
done with the data there.
And if write() ever gets implemented, that would be as well (but the io
iter stuff should cover that).
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-14 15:42 ` Greg KH
@ 2025-08-14 17:23 ` Benno Lossin
2025-08-14 18:26 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Benno Lossin @ 2025-08-14 17:23 UTC (permalink / raw)
To: Greg KH
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
> On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
>> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
>> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
>> >> I didn't have too much time to spend on this API, so this is mostly a
>> >> resend of v3. There are some changes in the last commit, updating to the
>> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
>> >> of v6.17-rc1.
>> >>
>> >> I think we should just merge the first two patches this cycle in order
>> >> to get the initial, bare-bones API into the kernel and have people
>> >> experiment with it. The validation logic in the third patch still needs
>> >> some work and I'd need to find some time to work on that (no idea when I
>> >> find it though).
>> >
>> > Nice, thanks for reviving this!
>> >
>> > And we should at least add an example using it, otherwise it's not going
>> > to help out much here. Add it to the misc device driver api?
>>
>> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
>> untrusted?
>
> mmap() is, but you can't do anything about that...
Which parameter is untrusted there and why can't I do anything about it?
> ioctl() is the callback that is taking untrusted data from userspace.
> That's one place we have had more kernel buffer overflows then I can
> count and ALWAYS needs to be properly verified before anything can be
> done with the data there.
Are the `cmd` & `arg` parameters the untrusted part? If so we probably
should have a single parameter so users can verify them at the same
time. Or am I thinking of the wrong thing to verify? (`file` should be
already in kernel memory, right?)
> And if write() ever gets implemented, that would be as well (but the io
> iter stuff should cover that).
Sounds good.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-14 17:23 ` Benno Lossin
@ 2025-08-14 18:26 ` Greg KH
2025-08-15 7:28 ` Benno Lossin
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2025-08-14 18:26 UTC (permalink / raw)
To: Benno Lossin
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Thu, Aug 14, 2025 at 07:23:45PM +0200, Benno Lossin wrote:
> On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
> > On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
> >> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
> >> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
> >> >> I didn't have too much time to spend on this API, so this is mostly a
> >> >> resend of v3. There are some changes in the last commit, updating to the
> >> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
> >> >> of v6.17-rc1.
> >> >>
> >> >> I think we should just merge the first two patches this cycle in order
> >> >> to get the initial, bare-bones API into the kernel and have people
> >> >> experiment with it. The validation logic in the third patch still needs
> >> >> some work and I'd need to find some time to work on that (no idea when I
> >> >> find it though).
> >> >
> >> > Nice, thanks for reviving this!
> >> >
> >> > And we should at least add an example using it, otherwise it's not going
> >> > to help out much here. Add it to the misc device driver api?
> >>
> >> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
> >> untrusted?
> >
> > mmap() is, but you can't do anything about that...
>
> Which parameter is untrusted there and why can't I do anything about it?
The whole memory range is untrusted as to what is written there, sorry,
it was a bad attempt at a joke, the kernel never gets a chance to know
what is happening.
> > ioctl() is the callback that is taking untrusted data from userspace.
> > That's one place we have had more kernel buffer overflows then I can
> > count and ALWAYS needs to be properly verified before anything can be
> > done with the data there.
>
> Are the `cmd` & `arg` parameters the untrusted part?
Yes, especially as `arg` is usually a pointer to "something".
> If so we probably
> should have a single parameter so users can verify them at the same
> time. Or am I thinking of the wrong thing to verify? (`file` should be
> already in kernel memory, right?)
Both are usually verified at different places, first `cmd` tells what
`arg` is going to be, and then the code goes off and parses whatever
`arg` points to (or contains for simple ioctls).
And for some, `arg` is just a place to write something back, so `arg`
needs no verification for them, it depends on what `cmd` is.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-14 18:26 ` Greg KH
@ 2025-08-15 7:28 ` Benno Lossin
2025-08-15 14:19 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Benno Lossin @ 2025-08-15 7:28 UTC (permalink / raw)
To: Greg KH
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Thu Aug 14, 2025 at 8:26 PM CEST, Greg KH wrote:
> On Thu, Aug 14, 2025 at 07:23:45PM +0200, Benno Lossin wrote:
>> On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
>> > On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
>> >> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
>> >> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
>> >> >> I didn't have too much time to spend on this API, so this is mostly a
>> >> >> resend of v3. There are some changes in the last commit, updating to the
>> >> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
>> >> >> of v6.17-rc1.
>> >> >>
>> >> >> I think we should just merge the first two patches this cycle in order
>> >> >> to get the initial, bare-bones API into the kernel and have people
>> >> >> experiment with it. The validation logic in the third patch still needs
>> >> >> some work and I'd need to find some time to work on that (no idea when I
>> >> >> find it though).
>> >> >
>> >> > Nice, thanks for reviving this!
>> >> >
>> >> > And we should at least add an example using it, otherwise it's not going
>> >> > to help out much here. Add it to the misc device driver api?
>> >>
>> >> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
>> >> untrusted?
>> >
>> > mmap() is, but you can't do anything about that...
>>
>> Which parameter is untrusted there and why can't I do anything about it?
>
> The whole memory range is untrusted as to what is written there, sorry,
> it was a bad attempt at a joke, the kernel never gets a chance to know
> what is happening.
Ahh that flew over my head :) So we'd either build `Untrusted` directly
into the `VmaNew`/`VmaRef` abstractions -- or if there is a way to have
a trusted `VmaNew`, we can wrap it in `mmap`.
>> > ioctl() is the callback that is taking untrusted data from userspace.
>> > That's one place we have had more kernel buffer overflows then I can
>> > count and ALWAYS needs to be properly verified before anything can be
>> > done with the data there.
>>
>> Are the `cmd` & `arg` parameters the untrusted part?
>
> Yes, especially as `arg` is usually a pointer to "something".
>
>> If so we probably
>> should have a single parameter so users can verify them at the same
>> time. Or am I thinking of the wrong thing to verify? (`file` should be
>> already in kernel memory, right?)
>
> Both are usually verified at different places, first `cmd` tells what
> `arg` is going to be, and then the code goes off and parses whatever
> `arg` points to (or contains for simple ioctls).
>
> And for some, `arg` is just a place to write something back, so `arg`
> needs no verification for them, it depends on what `cmd` is.
I still think grouping them together makes sense, since in the
validation function you'd want access to both, right? And these use
cases seem perfect for enums:
enum MyIoctlArgs {
WriteFoo(UserPtr<Foo>),
VerifyBar(UserPtr<Bar>),
// ...
}
And then in the validation function you can do:
const WRITE_FOO_CMD: u32 = ...;
fn validate(cmd: u32, arg: usize) -> Result<MyIoctlArgs> {
Ok(match cmd {
WRITE_FOO_CMD => MyIoctlArgs::WriteFoo(UserPtr::from_addr(arg)),
VERIFY_BAR_CMD => MyIoctlArgs::VerifyBar(UserPtr::from_addr(arg)),
_ => return Err(EINVAL),
})
}
So when wrapping them together we could have:
pub struct IoctlArgs {
pub cmd: u32,
pub arg: usize,
}
And then change the `MiscDevice::ioctl` function to:
fn ioctl(
_device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
_file: &File,
_args: Untrusted<IoctlArgs>,
) -> Result<isize>
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-15 7:28 ` Benno Lossin
@ 2025-08-15 14:19 ` Greg KH
2025-08-16 10:22 ` Benno Lossin
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2025-08-15 14:19 UTC (permalink / raw)
To: Benno Lossin
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Fri, Aug 15, 2025 at 09:28:59AM +0200, Benno Lossin wrote:
> On Thu Aug 14, 2025 at 8:26 PM CEST, Greg KH wrote:
> > On Thu, Aug 14, 2025 at 07:23:45PM +0200, Benno Lossin wrote:
> >> On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
> >> > On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
> >> >> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
> >> >> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
> >> >> >> I didn't have too much time to spend on this API, so this is mostly a
> >> >> >> resend of v3. There are some changes in the last commit, updating to the
> >> >> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
> >> >> >> of v6.17-rc1.
> >> >> >>
> >> >> >> I think we should just merge the first two patches this cycle in order
> >> >> >> to get the initial, bare-bones API into the kernel and have people
> >> >> >> experiment with it. The validation logic in the third patch still needs
> >> >> >> some work and I'd need to find some time to work on that (no idea when I
> >> >> >> find it though).
> >> >> >
> >> >> > Nice, thanks for reviving this!
> >> >> >
> >> >> > And we should at least add an example using it, otherwise it's not going
> >> >> > to help out much here. Add it to the misc device driver api?
> >> >>
> >> >> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
> >> >> untrusted?
> >> >
> >> > mmap() is, but you can't do anything about that...
> >>
> >> Which parameter is untrusted there and why can't I do anything about it?
> >
> > The whole memory range is untrusted as to what is written there, sorry,
> > it was a bad attempt at a joke, the kernel never gets a chance to know
> > what is happening.
>
> Ahh that flew over my head :) So we'd either build `Untrusted` directly
> into the `VmaNew`/`VmaRef` abstractions -- or if there is a way to have
> a trusted `VmaNew`, we can wrap it in `mmap`.
Nah, I wouldn't worry about that, mmap() doesn't seem to cause security
issues as by definition it is just allowing userspace to read/write
anything to that device or memory, and the kernel doesn't even see it.
Because of that, the kernel can't really be "broken" with invalid data
there (hardware can, of course, but that's userspace's fault the kernel
is just setting up a pipe here.)
> >> If so we probably
> >> should have a single parameter so users can verify them at the same
> >> time. Or am I thinking of the wrong thing to verify? (`file` should be
> >> already in kernel memory, right?)
> >
> > Both are usually verified at different places, first `cmd` tells what
> > `arg` is going to be, and then the code goes off and parses whatever
> > `arg` points to (or contains for simple ioctls).
> >
> > And for some, `arg` is just a place to write something back, so `arg`
> > needs no verification for them, it depends on what `cmd` is.
>
> I still think grouping them together makes sense, since in the
> validation function you'd want access to both, right? And these use
> cases seem perfect for enums:
>
> enum MyIoctlArgs {
> WriteFoo(UserPtr<Foo>),
> VerifyBar(UserPtr<Bar>),
> // ...
> }
>
> And then in the validation function you can do:
>
> const WRITE_FOO_CMD: u32 = ...;
>
> fn validate(cmd: u32, arg: usize) -> Result<MyIoctlArgs> {
> Ok(match cmd {
> WRITE_FOO_CMD => MyIoctlArgs::WriteFoo(UserPtr::from_addr(arg)),
> VERIFY_BAR_CMD => MyIoctlArgs::VerifyBar(UserPtr::from_addr(arg)),
> _ => return Err(EINVAL),
> })
> }
>
> So when wrapping them together we could have:
>
> pub struct IoctlArgs {
> pub cmd: u32,
> pub arg: usize,
> }
>
> And then change the `MiscDevice::ioctl` function to:
>
> fn ioctl(
> _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
> _file: &File,
> _args: Untrusted<IoctlArgs>,
> ) -> Result<isize>
I think the problem with this is you now end up with the "I verified
this is ok, so now I will copy it" bug, where userspace will race with
the kernel and modify the data after verification but before copying.
Note, Windows is full of these types of bugs as they don't do a call to
copy_from_user(), but usually just poke at the data directly. Linux at
least forces a copy_from_user() call, but it doesn't always work in that
you still have to validate the data is correct and people forget.
So as long as we can copy the data from userspace first, and then
validate it, and keep that validated copy around to use, that's great.
I can't really determine above if that is the case or not, sorry.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-15 14:19 ` Greg KH
@ 2025-08-16 10:22 ` Benno Lossin
2025-08-17 6:00 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Benno Lossin @ 2025-08-16 10:22 UTC (permalink / raw)
To: Greg KH
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Fri Aug 15, 2025 at 4:19 PM CEST, Greg KH wrote:
> On Fri, Aug 15, 2025 at 09:28:59AM +0200, Benno Lossin wrote:
>> On Thu Aug 14, 2025 at 8:26 PM CEST, Greg KH wrote:
>> > On Thu, Aug 14, 2025 at 07:23:45PM +0200, Benno Lossin wrote:
>> >> On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
>> >> > On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
>> >> >> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
>> >> >> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
>> >> >> >> I didn't have too much time to spend on this API, so this is mostly a
>> >> >> >> resend of v3. There are some changes in the last commit, updating to the
>> >> >> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
>> >> >> >> of v6.17-rc1.
>> >> >> >>
>> >> >> >> I think we should just merge the first two patches this cycle in order
>> >> >> >> to get the initial, bare-bones API into the kernel and have people
>> >> >> >> experiment with it. The validation logic in the third patch still needs
>> >> >> >> some work and I'd need to find some time to work on that (no idea when I
>> >> >> >> find it though).
>> >> >> >
>> >> >> > Nice, thanks for reviving this!
>> >> >> >
>> >> >> > And we should at least add an example using it, otherwise it's not going
>> >> >> > to help out much here. Add it to the misc device driver api?
>> >> >>
>> >> >> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
>> >> >> untrusted?
>> >> >
>> >> > mmap() is, but you can't do anything about that...
>> >>
>> >> Which parameter is untrusted there and why can't I do anything about it?
>> >
>> > The whole memory range is untrusted as to what is written there, sorry,
>> > it was a bad attempt at a joke, the kernel never gets a chance to know
>> > what is happening.
>>
>> Ahh that flew over my head :) So we'd either build `Untrusted` directly
>> into the `VmaNew`/`VmaRef` abstractions -- or if there is a way to have
>> a trusted `VmaNew`, we can wrap it in `mmap`.
>
> Nah, I wouldn't worry about that, mmap() doesn't seem to cause security
> issues as by definition it is just allowing userspace to read/write
> anything to that device or memory, and the kernel doesn't even see it.
> Because of that, the kernel can't really be "broken" with invalid data
> there (hardware can, of course, but that's userspace's fault the kernel
> is just setting up a pipe here.)
I'm guessing that the kernel doesn't usually read these? (if we don't
have a way to read them from the Rust side, we don't need `Untrusted`)
>> >> If so we probably
>> >> should have a single parameter so users can verify them at the same
>> >> time. Or am I thinking of the wrong thing to verify? (`file` should be
>> >> already in kernel memory, right?)
>> >
>> > Both are usually verified at different places, first `cmd` tells what
>> > `arg` is going to be, and then the code goes off and parses whatever
>> > `arg` points to (or contains for simple ioctls).
>> >
>> > And for some, `arg` is just a place to write something back, so `arg`
>> > needs no verification for them, it depends on what `cmd` is.
>>
>> I still think grouping them together makes sense, since in the
>> validation function you'd want access to both, right? And these use
>> cases seem perfect for enums:
>>
>> enum MyIoctlArgs {
>> WriteFoo(UserPtr<Foo>),
>> VerifyBar(UserPtr<Bar>),
>> // ...
>> }
>>
>> And then in the validation function you can do:
>>
>> const WRITE_FOO_CMD: u32 = ...;
>>
>> fn validate(cmd: u32, arg: usize) -> Result<MyIoctlArgs> {
>> Ok(match cmd {
>> WRITE_FOO_CMD => MyIoctlArgs::WriteFoo(UserPtr::from_addr(arg)),
>> VERIFY_BAR_CMD => MyIoctlArgs::VerifyBar(UserPtr::from_addr(arg)),
>> _ => return Err(EINVAL),
>> })
>> }
>>
>> So when wrapping them together we could have:
>>
>> pub struct IoctlArgs {
>> pub cmd: u32,
>> pub arg: usize,
>> }
>>
>> And then change the `MiscDevice::ioctl` function to:
>>
>> fn ioctl(
>> _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
>> _file: &File,
>> _args: Untrusted<IoctlArgs>,
>> ) -> Result<isize>
>
> I think the problem with this is you now end up with the "I verified
> this is ok, so now I will copy it" bug, where userspace will race with
> the kernel and modify the data after verification but before copying.
>
> Note, Windows is full of these types of bugs as they don't do a call to
> copy_from_user(), but usually just poke at the data directly. Linux at
> least forces a copy_from_user() call, but it doesn't always work in that
> you still have to validate the data is correct and people forget.
>
> So as long as we can copy the data from userspace first, and then
> validate it, and keep that validated copy around to use, that's great.
> I can't really determine above if that is the case or not, sorry.
So if I understand the current API correctly, the `arg` and `cmd`
parameters are copied from userspace (or rather they are direct
parameters of the syscall), so you can't have the copy-after-validation
bug there. Then in the `ioctl` function one currently just looks at
`cmd` & then decides what to do with `arg`, possibly doing a
`copy_from_user`.
In my suggestion for the API, we just change the first part of the
current approach, combining the two parameters to `ioctl` into a single
struct & making people parse it using the untrusted API.
The API that protects you from the copy-after-validation bug is the
`UserPtr<T>` abstraction. And there I also will add the untrusted API.
For example the `UserPtr::read_all` function would become:
pub fn read_all<A: Allocator>(self, buf: &mut Untrusted<Vec<u8, A>>, flags: Flags) -> Result;
This means that you can only read the data into an `Untrusted<Vec<u8>>`
which ensures that the data is validated before use. This API also makes
it harder to have the copy-after-validation bug, since you have to
explicitly call `clone_reader`.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 0/4] Untrusted Data API
2025-08-16 10:22 ` Benno Lossin
@ 2025-08-17 6:00 ` Greg KH
0 siblings, 0 replies; 16+ messages in thread
From: Greg KH @ 2025-08-17 6:00 UTC (permalink / raw)
To: Benno Lossin
Cc: Simona Vetter, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux
On Sat, Aug 16, 2025 at 12:22:04PM +0200, Benno Lossin wrote:
> On Fri Aug 15, 2025 at 4:19 PM CEST, Greg KH wrote:
> > On Fri, Aug 15, 2025 at 09:28:59AM +0200, Benno Lossin wrote:
> >> On Thu Aug 14, 2025 at 8:26 PM CEST, Greg KH wrote:
> >> > On Thu, Aug 14, 2025 at 07:23:45PM +0200, Benno Lossin wrote:
> >> >> On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
> >> >> > On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
> >> >> >> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
> >> >> >> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
> >> >> >> >> I didn't have too much time to spend on this API, so this is mostly a
> >> >> >> >> resend of v3. There are some changes in the last commit, updating to the
> >> >> >> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
> >> >> >> >> of v6.17-rc1.
> >> >> >> >>
> >> >> >> >> I think we should just merge the first two patches this cycle in order
> >> >> >> >> to get the initial, bare-bones API into the kernel and have people
> >> >> >> >> experiment with it. The validation logic in the third patch still needs
> >> >> >> >> some work and I'd need to find some time to work on that (no idea when I
> >> >> >> >> find it though).
> >> >> >> >
> >> >> >> > Nice, thanks for reviving this!
> >> >> >> >
> >> >> >> > And we should at least add an example using it, otherwise it's not going
> >> >> >> > to help out much here. Add it to the misc device driver api?
> >> >> >>
> >> >> >> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
> >> >> >> untrusted?
> >> >> >
> >> >> > mmap() is, but you can't do anything about that...
> >> >>
> >> >> Which parameter is untrusted there and why can't I do anything about it?
> >> >
> >> > The whole memory range is untrusted as to what is written there, sorry,
> >> > it was a bad attempt at a joke, the kernel never gets a chance to know
> >> > what is happening.
> >>
> >> Ahh that flew over my head :) So we'd either build `Untrusted` directly
> >> into the `VmaNew`/`VmaRef` abstractions -- or if there is a way to have
> >> a trusted `VmaNew`, we can wrap it in `mmap`.
> >
> > Nah, I wouldn't worry about that, mmap() doesn't seem to cause security
> > issues as by definition it is just allowing userspace to read/write
> > anything to that device or memory, and the kernel doesn't even see it.
> > Because of that, the kernel can't really be "broken" with invalid data
> > there (hardware can, of course, but that's userspace's fault the kernel
> > is just setting up a pipe here.)
>
> I'm guessing that the kernel doesn't usually read these? (if we don't
> have a way to read them from the Rust side, we don't need `Untrusted`)
Correct.
> >> >> If so we probably
> >> >> should have a single parameter so users can verify them at the same
> >> >> time. Or am I thinking of the wrong thing to verify? (`file` should be
> >> >> already in kernel memory, right?)
> >> >
> >> > Both are usually verified at different places, first `cmd` tells what
> >> > `arg` is going to be, and then the code goes off and parses whatever
> >> > `arg` points to (or contains for simple ioctls).
> >> >
> >> > And for some, `arg` is just a place to write something back, so `arg`
> >> > needs no verification for them, it depends on what `cmd` is.
> >>
> >> I still think grouping them together makes sense, since in the
> >> validation function you'd want access to both, right? And these use
> >> cases seem perfect for enums:
> >>
> >> enum MyIoctlArgs {
> >> WriteFoo(UserPtr<Foo>),
> >> VerifyBar(UserPtr<Bar>),
> >> // ...
> >> }
> >>
> >> And then in the validation function you can do:
> >>
> >> const WRITE_FOO_CMD: u32 = ...;
> >>
> >> fn validate(cmd: u32, arg: usize) -> Result<MyIoctlArgs> {
> >> Ok(match cmd {
> >> WRITE_FOO_CMD => MyIoctlArgs::WriteFoo(UserPtr::from_addr(arg)),
> >> VERIFY_BAR_CMD => MyIoctlArgs::VerifyBar(UserPtr::from_addr(arg)),
> >> _ => return Err(EINVAL),
> >> })
> >> }
> >>
> >> So when wrapping them together we could have:
> >>
> >> pub struct IoctlArgs {
> >> pub cmd: u32,
> >> pub arg: usize,
> >> }
> >>
> >> And then change the `MiscDevice::ioctl` function to:
> >>
> >> fn ioctl(
> >> _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
> >> _file: &File,
> >> _args: Untrusted<IoctlArgs>,
> >> ) -> Result<isize>
> >
> > I think the problem with this is you now end up with the "I verified
> > this is ok, so now I will copy it" bug, where userspace will race with
> > the kernel and modify the data after verification but before copying.
> >
> > Note, Windows is full of these types of bugs as they don't do a call to
> > copy_from_user(), but usually just poke at the data directly. Linux at
> > least forces a copy_from_user() call, but it doesn't always work in that
> > you still have to validate the data is correct and people forget.
> >
> > So as long as we can copy the data from userspace first, and then
> > validate it, and keep that validated copy around to use, that's great.
> > I can't really determine above if that is the case or not, sorry.
>
> So if I understand the current API correctly, the `arg` and `cmd`
> parameters are copied from userspace (or rather they are direct
> parameters of the syscall), so you can't have the copy-after-validation
> bug there. Then in the `ioctl` function one currently just looks at
> `cmd` & then decides what to do with `arg`, possibly doing a
> `copy_from_user`.
>
> In my suggestion for the API, we just change the first part of the
> current approach, combining the two parameters to `ioctl` into a single
> struct & making people parse it using the untrusted API.
>
> The API that protects you from the copy-after-validation bug is the
> `UserPtr<T>` abstraction. And there I also will add the untrusted API.
> For example the `UserPtr::read_all` function would become:
>
> pub fn read_all<A: Allocator>(self, buf: &mut Untrusted<Vec<u8, A>>, flags: Flags) -> Result;
>
> This means that you can only read the data into an `Untrusted<Vec<u8>>`
> which ensures that the data is validated before use. This API also makes
> it harder to have the copy-after-validation bug, since you have to
> explicitly call `clone_reader`.
Ok, that sounds good!
^ permalink raw reply [flat|nested] 16+ messages in thread