Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The overflow check `align_mask.checked_add(self.len())?.checked_add(nbits)?` incorrectly rejects valid `next_zero_area_off` queries on 32-bit architectures when `self.len()` and `nbits` are large. - [Medium] The assertion `start < self.len()` in `next_zero_area_off` panics if `start == self.len()`, which breaks iterators and empty bitmaps. --
--- Patch [3]: [PATCH v4 3/5] rust: bitmap: add contiguous area operations --- commit cd8a17b1e3284bd61e474e6a67facb0e46c2ec8a Author: Eliot Courtney <[email protected]> rust: bitmap: add contiguous area operations Add bindings for area operations on bitmaps. Each one is made safe by adding some extra checks compared to the underlying C code (for example, checking bounds) and with additional checks to catch likely erroneous usage if CONFIG_RUST_BITMAP_HARDENED is on. Tests demonstrating the edge cases are also included. > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs > index 0d481d761f2ac..a2557e9c5cfe0 100644 > --- a/rust/kernel/bitmap.rs > +++ b/rust/kernel/bitmap.rs [ ... ] > @@ -504,6 +504,138 @@ pub fn next_zero_bit(&self, start: usize) -> > Option<usize> { > Some(index) > } > } > + > + /// Finds a contiguous area of `nbits` zero bits at or after `start`, > where the area plus > + /// `align_offset` is aligned to `align`. > + /// > + /// Returns the bit index of the start of the area, or [`None`] if no > such area fitting in > + /// the bitmap exists. > + /// > + /// The returned index plus `align_offset` is a multiple of `align`. > + /// > + /// # Panics > + /// > + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out > of bounds. > + #[inline] > + pub fn next_zero_area_off( > + &self, > + start: usize, > + nbits: usize, > + align: Alignment, > + align_offset: usize, > + ) -> Option<usize> { > + bitmap_assert!( > + start < self.len(), [Severity: Medium] Does this assertion break iterators and empty bitmaps when CONFIG_RUST_BITMAP_HARDENED is enabled? If a caller iteratively searches for zero areas (e.g., start = area_index + nbits) and the last found area ends exactly at the bitmap boundary, the next iteration will query with start == self.len(). Also, querying an empty bitmap (self.len() == 0) with start = 0 would trigger this panic. The underlying C function bitmap_find_next_zero_area_off() safely processes start >= size by returning size, which the Rust code gracefully transforms to None. Could we use start <= self.len() here instead, similar to how the set() and clear() functions allow end <= self.len()? > + "`start` must be < {}, was {}", > + self.len(), > + start > + ); > + > + let nr = u32::try_from(nbits).ok()?; > + > + // The C alignment and end arithmetic must not overflow, or it can > read out of bounds. > + // Overflow is only possible on 32-bit. > + let align_mask = align.as_usize() - 1; > + align_mask.checked_add(self.len())?.checked_add(nbits)?; [Severity: Medium] Is this overflow check overly strict for 32-bit architectures? If a caller requests a large contiguous area in a large bitmap, such as self.len() = 0x7FFFFFFF and nbits = 0x7FFFFFFF, the sum of align_mask + self.len() + nbits can exceed u32::MAX. This would cause the checked_add() chain to fail and incorrectly return None instead of executing the search. Since we only need to prevent the C function's end = index + nr calculation from wrapping around maliciously, and we know nbits <= self.len() makes nr at most 0x7FFFFFFF, any wrap-around of index + nr on 32-bit would mathematically result in an end value strictly less than index. The C code handles end < index safely by returning index, which Rust then correctly rejects since index < self.len() is false. Would a simpler check like "if nbits > self.len() { return None; }" be sufficient to prevent malicious wrap-around while preserving valid searches for large boundaries? > + > + // SAFETY: `bitmap_find_next_zero_area_off` is safe to use with an > out of bounds `start` > + // value and, given the overflow check above, never reads beyond > `self.len()` bits. > + let index = unsafe { > + bindings::bitmap_find_next_zero_area_off( > + self.as_ptr().cast_mut(), [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
