Joel Fernandes
2025-Sep-30 22:06 UTC
[PATCH v5 8/9] rust: bitfield: Add hardening for out of bounds access
Hello, Yury, On 9/30/2025 2:03 PM, Yury Norov wrote:> 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 at gmail.com> >> Signed-off-by: Joel Fernandes <joelagnelf at 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.Sure, so the cost is zero if either CONFIG_RUST_BITFIELD_HARDENED is disabled or the value being checked is a constant. That is, the compiler eliminates the dead code. Otherwise the cost is a single shift instruction and a single conditional jump on x86. I verified this. As such, I think it is a good idea to keep the check on.> >> /// 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.Yes, that case is a compile time error.> Can you> elaborate under which condition this check makes sense, and is not > covered with the "(1<<BITS) - 1" case?If BITS is 64, then the else should not execute, because it would become (1 << 64) which is UB. I am Ok with changing the condition to: "if BITS =(::core::mem::size_of::<$storage>() * 8)" considering the compiler time error, but I am also Ok with leaving it as it is.> >> + !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".No, this a compile-time issue. Even if there is no overflow, you cannot use '::from()' because it will cause a compile-time error because it is narrowing potentially a wider width (like u32) to a narrower width (like u8). It is not so much about the value itself, as much as it is about the type of 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.Why? That's more unreadable IMO. Having the condition itself in the assert, clearly shows what the condition is in the context of the surrounding code. I don't think we should more macros to do asserts when we already have a macro.> >> + >> + 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.There's no way to do that other than always panicking, or introducing a new API. There's also no way to do this at compile time, because 'value' may not always be a constant. I see your point, but I think since we already panicking at runtime in a hardened config (or printing an error otherwise), we already have that covered right? The user is already notified and it does not go by silently. If this is really a problem, we can always panic (in other words, always keep it as hardened config?).> > This "chain of setters" thing looks weird to me as I already said. So > if it messes with a clear API, just drop it.I don't think we can/should drop that because this library is also used by the register macro, and that uses the setter API (I just moved this code from there). Also builder pattern is a common paradigm in Rust programs and I'd say one of the key features of the bitfield macro so we should support it. How else will you do something like: let status2 = TestStruct::default() .set_ready(true) .set_state(0x2) .set_reserved(0x5); ? :-) As an aside, you can check literature on the builder pattern as well, it keeps code super clean and readable.> > And to that extend, > > a = a.set_field1() > > looks more questionable than just > > a.set_field1() > > because it implies an extra copy.a.set_field1() on its own wont work, because the value returned (self) is discarded. and, a = a.set_field1() a = a.set_field2() a = a.set_field3() doesn't really do multiple copies, the compiler optimizes things and eliminates copies. I verified this in the asm as well.> 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()'?You can't do that in the current implementation, it is a NO-OP. We can certainly add a new API for that, initially I was planning to do add a 'with_field*()' accessor for the builder pattern, and do the 'set_field*()' API as you mentioned. So it would look like: let status2 = TestStruct::default() .with_ready(true) .with_state(0x2) .with_reserved(0x5); Or one could do: let status2 = TestStruct::default(); status2.set_ready(true); status2.set_state(0x2); status2.set_reserved(0x5); Would that address your concern?> Can you ensure it holds, say, on 32-bit machine when> 'a' is a 64-bit bitfield?I don't think this is an issue. If the user writes a = a.set_fieldXX() multiple times, it will be optimized as I mentioned above. I have verified (in release builds) that the compiler does not create multiple intermediate copies.> Would it work if we decide to support bitfields larger than 64-bit, like C does?Certainly, the struct has the Copy trait. 'a = a.set_field()' will work for larger than 64-bit just fine, if/when we support it. I verified that as well. thanks, - Joel> 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