Re: [PATCH v5 2/8] nvmem: core: Enforce stride and alignment checks for nvmem_device functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unconditionally enforcing `word_size` alignment in `nvmem_device_read` breaks existing in-kernel consumers and backend drivers that safely support unaligned reads.
- [Medium] The `IS_ALIGNED` macro is incorrectly used with `nvmem->stride` and `nvmem->word_size`, which are not guaranteed to be powers of two.

Pre-existing issues:
- [High] Missing bounds checking in `nvmem_device_read` and `nvmem_device_write` against `nvmem->size` exposes the system to out-of-bounds memory accesses.
- [High] Backend drivers perform SLAB out-of-bounds reads when processing writes for bit-offset cells due to mismatched buffer sizes.
--

commit d59bc7c372de847ac014e9a361f8f7bf075ff75c
Author: Gregor Herburger <gregor.herburger@linutronix.de>

nvmem: core: Enforce stride and alignment checks for nvmem_device functions

This commit adds alignment checks for offset and bytes against stride and
word_size to the nvmem_device_read and nvmem_device_write functions.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 311cb2e5a5c02..6b313f63d07ef 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -2068,6 +2068,12 @@ int nvmem_device_read(struct nvmem_device *nvmem,
>  	if (!nvmem)
>  		return -EINVAL;
>  
> +	if (!IS_ALIGNED(offset, nvmem->stride))
> +		return -EINVAL;
> +
> +	if (!IS_ALIGNED(bytes, nvmem->word_size))
> +		return -EINVAL;

[Severity: Medium]
Is the IS_ALIGNED() macro safe to use here? Looking at include/vdso/align.h,
IS_ALIGNED(x, a) is implemented using bitwise arithmetic:
(((x) & ((typeof(x))(a) - 1)) == 0)

This macro is mathematically valid only when the alignment is a power of two.
Since the nvmem subsystem doesn't enforce power-of-two constraints for stride
or word_size during registration, if a driver defines a non-power-of-two
value, wouldn't this macro evaluate incorrectly and reject valid alignments?

[Severity: High]
Could this unconditional word_size check break existing backend drivers that
safely support unaligned reads?

Historically, nvmem_device_read() permitted unaligned lengths, delegating the
read to the backend's reg_read function. Since the framework lacks separate
read and write constraints, many drivers specify a larger word_size to
enforce constraints strictly on writes, but gracefully handle arbitrary-sized
unaligned reads internally (e.g., using bounce buffers). 

Returning -EINVAL here might break legitimate read operations for dynamic
payload lengths.

>  	rc = nvmem_reg_read(nvmem, offset, buf, bytes);

[Severity: High]
This is a pre-existing issue, but does this function need bounds checking
against nvmem->size?

Currently, nvmem_device_read() passes the unchecked offset directly to backend
callbacks. If an in-kernel caller provides an out-of-bounds offset, many
hardware driver callbacks lack complete internal bounds checking, which might
lead to arbitrary out-of-bounds MMIO accesses or integer underflow issues.

[ ... ]

> @@ -2096,6 +2102,12 @@ int nvmem_device_write(struct nvmem_device *nvmem,
>  	if (!nvmem)
>  		return -EINVAL;
>  
> +	if (!IS_ALIGNED(offset, nvmem->stride))
> +		return -EINVAL;
> +
> +	if (!IS_ALIGNED(bytes, nvmem->word_size))
> +		return -EINVAL;
> +
>  	rc = nvmem_reg_write(nvmem, offset, buf, bytes);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for backend
drivers to perform SLAB out-of-bounds reads when processing writes for
bit-offset cells?

While this patch enforces word_size aligned lengths on direct nvmem writes,
__nvmem_cell_entry_write() bypasses these checks and calls nvmem_reg_write()
with a dynamically allocated buffer that is exactly cell->bytes long.

If a bit-offset cell spans 1 byte, but the backend driver assumes all
operations are word_size aligned (e.g., casting buf to a u32 pointer and
writing 4 bytes), could the backend perform a SLAB out-of-bounds read by
dereferencing 4 bytes from a 1-byte heap allocation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-rpi-otp-driver-v5-0-b26e5908eeac@linutronix.de?part=2