Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] SysMemBackend::copy_read and copy_write use read_volatile and write_volatile on arbitrary generic types T, violating LKMM and LLVM constraints. - [High] MmioBackend::copy_from_io and copy_to_io map bulk copies to memcpy_fromio/memcpy_toio, violating endianness subsystem guidelines. - [High] SysMemBackend::copy_from_io and copy_to_io rely on bindings::memcpy for volatile semantics, leading to TOCTOU and data races. --
commit 8004726b3c24d7011df864d4b9595e8a500deac8 Author: Gary Guo <[email protected]> rust: io: re-introduce copy methods This commit re-introduces the ability to read and write large structs that was lost when migrating to the io_read and io_write macros. It adds copying methods that model memcpy semantics to ensure users expect a lack of atomicity. It also extends these methods for MMIO via memcpy_fromio and memcpy_toio. diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs index f64ca2202ff2..16f818d396bf 100644 --- a/rust/kernel/io.rs +++ b/rust/kernel/io.rs [ ... ] > @@ -981,6 +1154,28 @@ fn io_write(view: <$backend as IoBackend>::View<'_, > $ty>, value: $ty) { > #[cfg(CONFIG_64BIT)] > impl_mmio_io_capable!(MmioBackend, u64, readq, writeq); > > +impl IoCopyable for MmioBackend { > + #[inline] > + unsafe fn copy_from_io(view: Self::View<'_, [u8]>, buffer: *mut u8) { > + // SAFETY: > + // - `view.ptr` is valid MMIO memory for `view.size()` bytes. > + // - `buffer` is valid for write for `view.size()` bytes. > + unsafe { > + bindings::memcpy_fromio(buffer.cast(), view.ptr.cast(), > view.size()); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Will this implementation violate the endianness subsystem guidelines for MmioBackend? The MmioBackend implements copy_from_io using memcpy_fromio, which is a stream accessor that copies memory byte-for-byte without performing CPU-to-device endianness byteswapping. In contrast, scalar MMIO accessors (like io_read/io_write using readl/writel) perform implicit byteswapping. If a user reads a hardware struct using copy_read, multi-byte integer fields will retain the device's byte order on big-endian systems, whereas reading them individually via io_read would swap them to native CPU endianness. Mixing stream and register accessors for the same memory region can lead to silent data corruption on big-endian architectures. > + } > + } > + > + #[inline] > + unsafe fn copy_to_io(view: Self::View<'_, [u8]>, buffer: *const u8) { > + // SAFETY: > + // - `view.ptr` is valid MMIO memory for `view.size()` bytes. > + // - `buffer` is valid for read for `view.size()` bytes. > + unsafe { > + bindings::memcpy_toio(view.ptr.cast(), buffer.cast(), > view.size()); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Could using memcpy_toio here also bypass endianness byteswapping? Using a stream accessor for a generic bulk copy instead of scalar register accessors might lead to endianness portability bugs when used alongside io_write on the same memory region. > + } > + } > +} [ ... ] > @@ -1145,6 +1340,47 @@ fn io_write(view: SysMem<'_, $ty>, value: $ty) { > #[cfg(CONFIG_64BIT)] > impl_sysmem_io_capable!(u64); > > +impl IoCopyable for SysMemBackend { > + #[inline] > + unsafe fn copy_from_io(view: Self::View<'_, [u8]>, buffer: *mut u8) { > + // Use bindings::memcpy instead of copy_nonoverlapping for volatile. > + // SAFETY: > + // - `view.ptr` is in CPU address space and valid for read. > + // - `buffer` is valid for write for `view.size()` bytes which is > equal to `view.ptr.len()`. > + unsafe { bindings::memcpy(buffer.cast(), view.ptr.cast(), > view.ptr.len()) }; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Is it safe to assume bindings::memcpy provides volatile access guarantees? The comment states it uses bindings::memcpy instead of copy_nonoverlapping for volatile, but bindings::memcpy provides no volatile guarantees in C or LLVM. Because memcpy is a standard memory operation, LLVM can aggressively optimize it, potentially eliding the copy if the destination is unread locally, reordering accesses, or tearing reads and writes to DMA coherent memory. This could strip the required volatile semantics and cause hardware desynchronization or TOCTOU vulnerabilities. > + } > + > + #[inline] > + unsafe fn copy_to_io(view: Self::View<'_, [u8]>, buffer: *const u8) { > + // Use bindings::memcpy instead of copy_nonoverlapping for volatile. > + // SAFETY: > + // - `view.ptr` is in CPU address space and valid for write. > + // - `buffer` is valid for read for `view.size()` bytes which is > equal to `view.ptr.len()`. > + unsafe { bindings::memcpy(view.ptr.cast(), buffer.cast(), > view.ptr.len()) }; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Might this copy also lack volatile guarantees for the same reason? Calling bindings::memcpy does not ensure volatile memory access semantics, which could lead to LLVM optimizing away the copy or reordering it. > + } > + > + #[inline] > + fn copy_read<T: FromBytes>(view: Self::View<'_, T>) -> T { > + // SAFETY: > + // - Per type invariant, ptr is valid and aligned. > + // - Using read_volatile() here so that race with hardware is > well-defined. > + // - Using read_volatile() here is not sound if it races with other > CPU per Rust > + // rules, but this is allowed per LKMM. > + // - The macro is only used on primitives so all bit patterns are > valid. > + unsafe { view.ptr.read_volatile() } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Can calling read_volatile() on a generic type T violate LKMM and LLVM constraints? Since copy_read is generic over T: FromBytes, T can be an arbitrarily large structure. The Linux Kernel Memory Model (LKMM) restricts volatile accesses to machine-word sizes or smaller. Using read_volatile on large structures can lead to unpredictable LLVM codegen, including non-volatile block copies or compiler crashes. The safety comment mentions that the macro is only used on primitives, which seems incorrectly copied from primitive accessors. > + } > + > + #[inline] > + fn copy_write<T: IntoBytes>(view: Self::View<'_, T>, value: T) { > + // SAFETY: > + // - Per type invariant, ptr is valid and aligned. > + // - Using write_volatile() here so that race with hardware is > well-defined. > + // - Using write_volatile() here is not sound if it races with other > CPU per Rust > + // rules, but this is allowed per LKMM. > + unsafe { view.ptr.write_volatile(value) } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Are there similar LKMM constraint violations when using write_volatile() on generic type T? Similar to copy_read, T could be larger than a machine word, which is not supported by LKMM for volatile writes and may cause unpredictable compiler behavior. > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=19
