Re: [PATCH 2/2] nvmem: core: add byte-mask support for non-contiguous cell bytes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] nvmem: core: byte_mask raw_len reads full device and rounds
  past size
- [Medium] nvmem: core: byte_mask validation bypassed when nbits is
  non-zero
- [Medium] nvmem: core: missing zero-fill of trailing bytes in
  __nvmem_cell_read()
--

commit 2da8dac9e0c0313c6b2e42597b707a19ff863b3a
Author: Hongmei Xu <hongmei.xu@oss.qualcomm.com>

nvmem: core: add byte-mask support for non-contiguous cell bytes

This commit adds support for a byte-mask cell property that selects and packs
non-contiguous bytes from an NVMEM region into the logical cell value.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 7d4cce8e27424..6c9994abce39f 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c

[ ... ]

> @@ -603,12 +645,19 @@ static int nvmem_cell_info_to_nvmem_cell_entry_nodup(struct nvmem_device *nvmem,
>  
>  	cell->bit_offset = info->bit_offset;
>  	cell->nbits = info->nbits;
> +	cell->byte_mask = info->byte_mask;
>  	cell->np = info->np;
>  
>  	if (cell->nbits) {
>  		cell->bytes = DIV_ROUND_UP(cell->nbits + cell->bit_offset,
>  					   BITS_PER_BYTE);
>  		cell->raw_len = ALIGN(cell->bytes, nvmem->word_size);
> +	} else if (cell->byte_mask) {
> +		int rc = nvmem_validate_byte_mask(nvmem, info);
> +
> +		if (rc)
> +			return rc;

[Severity: Medium]
Does placing nvmem_validate_byte_mask() in the else-if branch after
if (cell->nbits) bypass validation when both nbits and byte_mask are set?

cell->byte_mask is copied unconditionally above, so if info->nbits is
non-zero, nvmem_cell_info_to_nvmem_cell_entry_nodup() takes the
if (cell->nbits) branch and skips nvmem_validate_byte_mask(). In
nvmem_add_cells_from_dt(), nvmem_validate_byte_mask() runs before
nvmem->fixup_dt_cell_info(), which can set info.nbits and info.bit_offset and
adjust info.offset (as qfprom_fixup_dt_cell_info() does when a cell offset is
not 4-byte aligned) before nvmem_add_one_cell() runs.

During __nvmem_cell_read(), nvmem_reg_read() then reads into rawbuf while
nvmem_shift_read_buffer_in_place() shifts the unwritten buf, and the
byte_mask loop writes hweight32(cell->byte_mask) bytes into buf without
bounds validation, corrupting the cell data or overflowing buf.

> +		cell->raw_len = nvmem->size - cell->offset;
>  	}

[Severity: High]
Can setting cell->raw_len to nvmem->size - cell->offset cause out-of-bounds
reads past nvmem->size?

Right below this block, nvmem_cell_info_to_nvmem_cell_entry_nodup() aligns
cell->raw_len up to nvmem->word_size:

drivers/nvmem/core.c:nvmem_cell_info_to_nvmem_cell_entry_nodup() {
    ...
	if (!IS_ALIGNED(cell->raw_len, nvmem->word_size)) {
		dev_err(&nvmem->dev,
			"cell %s raw len %zd unaligned to nvmem word size %d\n",
			cell->name ?: "<unknown>", cell->raw_len,
			nvmem->word_size);

		if (info->raw_len)
			return -EINVAL;

		cell->raw_len = ALIGN(cell->raw_len, nvmem->word_size);
	}
    ...
}

When nvmem->size - cell->offset is not a multiple of nvmem->word_size,
rounding cell->raw_len up makes cell->offset + cell->raw_len exceed
nvmem->size, causing __nvmem_cell_read() to call nvmem_reg_read() past the
end of the device.

Even when aligned, setting cell->raw_len to the rest of the device rather
than the masked span (at most 32 bytes) forces every read to allocate and
read the entire trailing region, which fails on providers where reading
protected or unmapped trailing fuse rows returns an error (such as
sec-qfprom).

[ ... ]

> @@ -1648,28 +1708,51 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
>  			     struct nvmem_cell_entry *cell,
>  			     void *buf, size_t *len, const char *id, int index)
>  {
> +	u8 *rawbuf = NULL;
> +	void *readbuf = buf;
>  	int rc;
>  
> -	rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->raw_len);
> +	if (cell->byte_mask) {
> +		rawbuf = kzalloc(cell->raw_len, GFP_KERNEL);
> +		if (!rawbuf)
> +			return -ENOMEM;
> +		readbuf = rawbuf;
> +	}
> +
> +	rc = nvmem_reg_read(nvmem, cell->offset, readbuf, cell->raw_len);
>  
>  	if (rc)
> -		return rc;
> +		goto err_exit;
>  
>  	/* shift bits in-place */
>  	if (cell->bit_offset || cell->nbits)
>  		nvmem_shift_read_buffer_in_place(cell, buf);
>  
> +	/* compact non-contiguous bytes selected by byte_mask */
> +	if (cell->byte_mask) {
> +		int out_idx = 0;
> +		int i;
> +
> +		for (i = 0; i < cell->raw_len && i < 32; i++) {
> +			if (cell->byte_mask & BIT(i))
> +				((u8 *)buf)[out_idx++] = rawbuf[i];
> +		}
> +	}

[Severity: Medium]
When hweight32(cell->byte_mask) is smaller than cell->bytes, does this leave
the trailing bytes in buf uninitialized instead of zero-filling them?

Because nvmem_reg_read() reads into rawbuf, buf is only written up to
out_idx. Callers such as nvmem_device_cell_read() pass a caller-supplied
buffer directly into __nvmem_cell_read() without pre-zeroing it and return
cell->bytes, leaving stale or uninitialized memory in
buf[out_idx .. cell->bytes - 1] for both cell->read_post_process() and the
caller.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-nvmem-byte-mask-for-split-macaddr-v1-0-138f715f18c1@oss.qualcomm.com?part=2