Re: [PATCH v5 8/9] rust: bitfield: Add hardening for out of bounds access

[Yury Norov <yury.norov@gmail.com>]

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@gmail.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.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