linux-kernel - Re: [PATCH v3 3/4] rust: validate: add `Validate` trait

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: <tencent_D04FE925ED0AD6F0A4C297CA1A1A74FDE608@qq.com>
Date: Tue, 22 Apr 2025 00:47:15 +0800
From: Guangbo Cui <2407018371@...com>
To: benno.lossin@...ton.me
Cc: a.hindborg@...nel.org,
	alex.gaynor@...il.com,
	aliceryhl@...gle.com,
	bjorn3_gh@...tonmail.com,
	boqun.feng@...il.com,
	gary@...yguo.net,
	gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org,
	ojeda@...nel.org,
	rust-for-linux@...r.kernel.org,
	simona.vetter@...ll.ch,
	tmgross@...ch.edu
Subject: Re: [PATCH v3 3/4] rust: validate: add `Validate` trait

Really cool patch! I got a few thoughts below.

> 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@...ton.me>
> ---
>  rust/kernel/validate.rs | 61 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/validate.rs b/rust/kernel/validate.rs
> index 8b33716be0c7..eecac39365db 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)
> +    }
>  }

The `validate_ref` and `validate_mut` functions should just call `V::validate(self)`
directly, since self is an Untrusted<T>, and you already implemented ValidateInput for it.
Calling `V::validate(&self.0)` would cause a type mismatch error.

> +/// Marks valid input for the [`Validate`] trait.
> +pub trait ValidateInput: private::Sealed {}
> +
> +impl<T: ?Sized> ValidateInput for Untrusted<T> {}
> +
> +impl<'a, T: ?Sized> ValidateInput for &'a Untrusted<T> {}
> +
> +impl<'a, T: ?Sized> ValidateInput for &'a mut Untrusted<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) -> Result<Self, Self::Err>;
> +}

Best regards,
Guangbo Cui