[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aNwbeQ6iXuKIsQHK@yury>
Date: Tue, 30 Sep 2025 14:03:37 -0400
From: Yury Norov <yury.norov@...il.com>
To: Joel Fernandes <joelagnelf@...dia.com>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
dri-devel@...ts.freedesktop.org, dakr@...nel.org,
acourbot@...dia.com, Alistair Popple <apopple@...dia.com>,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
bjorn3_gh@...tonmail.com, Benno Lossin <lossin@...nel.org>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
joel@...lfernandes.org, Elle Rhumsaa <elle@...thered-steel.dev>,
Daniel Almeida <daniel.almeida@...labora.com>,
Andrea Righi <arighi@...dia.com>, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v5 8/9] rust: bitfield: Add hardening for out of bounds
access
On Tue, Sep 30, 2025 at 10:45:36AM -0400, Joel Fernandes wrote:
> Similar to bitmap.rs, add hardening to print errors or assert if the
> setter API is used to write out-of-bound values.
>
> Suggested-by: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
> ---
> rust/kernel/bitfield.rs | 32 +++++++++++++++++++++++++++++++-
> security/Kconfig.hardening | 9 +++++++++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
> index a74e6d45ecd3..655f940479f1 100644
> --- a/rust/kernel/bitfield.rs
> +++ b/rust/kernel/bitfield.rs
> @@ -29,6 +29,20 @@ pub const fn into_raw(self) -> T {
> }
> }
>
> +/// Assertion macro for bitfield
> +#[macro_export]
> +macro_rules! bitfield_assert {
> + ($cond:expr, $($arg:tt)+) => {
> + #[cfg(CONFIG_RUST_BITFIELD_HARDENED)]
> + ::core::assert!($cond, $($arg)*);
> +
> + #[cfg(not(CONFIG_RUST_BITFIELD_HARDENED))]
> + if !($cond) {
> + $crate::pr_err!($($arg)+);
> + }
> + }
> +}
Can you discuss performance implication? I'm OK if you decided to make
the check always on, but we need to understand the cost of it.
> /// Bitfield macro definition.
> /// Define a struct with accessors to access bits within an inner unsigned integer.
> ///
> @@ -358,9 +372,25 @@ impl $name {
> $vis fn [<set_ $field>](mut self, value: $to_type) -> Self {
> const MASK: $storage = $name::[<$field:upper _MASK>];
> const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> + const BITS: u32 = ($hi - $lo + 1) as u32;
> + const MAX_VALUE: $storage =
> + if BITS >= (::core::mem::size_of::<$storage>() * 8) as u32 {
If BITS > storage then it should be a compile time error. Can you
elaborate under which condition this check makes sense, and is not
covered with the "(1<<BITS) - 1" case?
> + !0
> + } else {
> + (1 << BITS) - 1
> + };
> +
> + // Check for overflow - value should fit within the field's bits.
> // Here we are potentially narrowing value from a wider bit value
> // to a narrower bit value. So we have to use `as` instead of `::from()`.
The new comment sounds opposite to the old one: if you check for
overflow, then there's no chance to "potentially narrow the value".
This "potentially" wording simply means undefined behavior.
> - let val = ((value as $storage) << SHIFT) & MASK;
> + let raw_field_value = value as $storage;
> +
> + $crate::bitfield_assert!(
> + raw_field_value <= MAX_VALUE,
> + "value {} exceeds {} for a {} bit field", raw_field_value, MAX_VALUE, BITS
> + );
Can you hide all the internals in the assertion function? Like:
$crate::bitfield_set_assert!(bitfield, field, value, "your message", ...);
We don't need assertion implementation in the main function body.
> +
> + let val = (raw_field_value << SHIFT) & MASK;
> let new_val = (self.raw() & !MASK) | val;
> all the internals in the assertion self.0 = ::kernel::bitfield::BitfieldInternalStorage::from_raw(new_val);
User wants to set an inappropriate value, and you know that because
you just have tested for it. But here you're accepting a definitely
wrong request. This doesn't look right.
On previous rounds you said you can't fail in setter because that
would break the "chain of setters" design. I understand that, but I
think that it's more important to have a clear defensive API that
returns an error when people do wrong things.
So please either find a way to return an error from the setter, or
some other mechanism to abort erroneous request and notify the user.
This "chain of setters" thing looks weird to me as I already said. So
if it messes with a clear API, just drop it.
And to that extend,
a = a.set_field1()
looks more questionable than just
a.set_field1()
because it implies an extra copy. If I do
b = a.set_field1()
would it change the content of 'a'?
Can I do just 'a.set_field1()'? There's no such an example in your
test.
Is that 'a = a.set_field()' thing really a zero-cost comparing to just
'a.set_field()'? Can you ensure it holds, say, on 32-bit machine when
'a' is a 64-bit bitfield? Would it work if we decide to support
bitfields larger than 64-bit, like C does?
Thanks,
Yury
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index 86f8768c63d4..e9fc6dcbd6c3 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -265,6 +265,15 @@ config RUST_BITMAP_HARDENED
>
> If unsure, say N.
>
> +config RUST_BITFIELD_HARDENED
> + bool "Check integrity of bitfield Rust API"
> + depends on RUST
> + help
> + Enables additional assertions in the Rust Bitfield API to catch
> + values that exceed the bitfield bounds.
> +
> + If unsure, say N.
> +
> config BUG_ON_DATA_CORRUPTION
> bool "Trigger a BUG when data corruption is detected"
> select LIST_HARDENED
> --
> 2.34.1
Powered by blists - more mailing lists