Re: [PATCH v3 02/19] rust: io: add missing safety requirement in `IoCapable` methods

[sashiko-bot@kernel.org]

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 <gary@garyguo.net>

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/20260608-io_projection-v3-0-c5cde13a5ec4@garyguo.net?part=2