Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Compile-time and runtime alignment checks in `Io` diverge, leading to inconsistent validation for types where size exceeds alignment. --
commit 5d5fe99db5fb0c140eaa13601726daff7a857423 Author: Gary Guo <[email protected]> rust: io: add missing safety requirement in IoCapable methods This commit updates the safety comments for io_read and io_write to cover alignment requirements. It also expands the Io validation checks to take the base address into account when verifying that an offset is aligned. > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs > index dcf3b40ffa48f..a58fb9a21dcad 100644 > --- a/rust/kernel/io.rs > +++ b/rust/kernel/io.rs > @@ -197,13 +197,15 @@ pub fn maxsize(&self) -> usize { > #[repr(transparent)] > pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>); > > -/// Checks whether an access of type `U` at the given `offset` > +/// Checks whether an access of type `U` at the given `base` and the given > `offset` > /// is valid within this region. > +/// > +/// The `base` is used for alignment checking only. This can be set to 0 to > skip the check. > #[inline] > -const fn offset_valid<U>(offset: usize, size: usize) -> bool { > +const fn offset_valid<U>(base: usize, offset: usize, size: usize) -> bool { > let type_size = core::mem::size_of::<U>(); > if let Some(end) = offset.checked_add(type_size) { > - end <= size && offset % type_size == 0 > + end <= size && (base.wrapping_add(offset) % type_size == 0) [Severity: High] Does this logic incorrectly use the type size instead of its alignment requirement? If a type has a size greater than its alignment, such as u64 on 32-bit architectures or custom array types, checking modulo type_size demands an overly strict alignment. This could force an array access to require a base address aligned to the size of the entire array. > } else { > false > } [ ... ] > @@ -311,7 +315,11 @@ pub trait Io { > // Always inline to optimize out error path of `build_assert`. > #[inline(always)] > fn io_addr_assert<U>(&self, offset: usize) -> usize { > - build_assert!(offset_valid::<U>(offset, Self::Target::MIN_SIZE)); > + // We cannot check alignment with `offset_valid` using > `self.addr()`. So set 0 for it and > + // ensure alignment by checking that the alignment of `U` is smaller > or equal to the > + // alignment of `Self::Target`. > + const_assert!(Alignment::of::<U>().as_usize() <= > Self::Target::MIN_ALIGN.as_usize()); > + build_assert!(offset_valid::<U>(0, offset, Self::Target::MIN_SIZE)); [Severity: High] Does bypassing the base check here create a divergence between the compile-time and runtime alignment checks? Because offset_valid uses type_size rather than alignment, passing 0 for the base here will cause the compile-time check to evaluate offset % size_of::<U>(). However, the runtime check in io_addr uses the true base address. For example, if base is 4 and offset is 8 for an 8-byte sized type with 4-byte alignment, this compile-time check passes (8 % 8 == 0) while the runtime check would fail ((4 + 8) % 8 != 0). Conversely, if base is 4 and offset is 4, the compile-time check fails (4 % 8 != 0) but the runtime check passes. Should offset_valid be updated to use align_of::<U>() to resolve both this divergence and the overly strict array alignment constraint? > > self.addr() + offset > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
