Re: [PATCH v4 3/3] nvmem: make all reg_write callbacks take const void *

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch claims to 'make all reg_write callbacks take const void *' and 'fix all users of this API', but it fails to migrate many nvmem drivers.

Pre-existing issues:
- [Critical] The `sprd_efuse_write` function reads out-of-bounds for small writes and silently truncates large writes, while `sprd_efuse_read` suffers from a stack buffer overflow.
- [High] The `bcm_otpc_write` function performs an out-of-bounds read when `bytes` is not a multiple of `otpc_row_size * word_size`.
- [High] The `lpc18xx_eeprom_gather_write` and `lpc18xx_eeprom_read` functions will enter an infinite loop and cause OOB memory access if `bytes` is not a multiple of 4.
--

--- Patch [3]: [PATCH v4 3/3] nvmem: make all reg_write callbacks take const void * ---
commit 4f1bd94da9f8c5ba3e21c869d089a1a71eeccbc7
Author: Link Mauve <linkmauve@linkmauve.fr>

nvmem: make all reg_write callbacks take const void *

The previous commit switched from a pointer to mutable data to a pointer
to immutable data, so let’s fix all users of this API.

> nvmem: make all reg_write callbacks take const void *
>
> The previous commit switched from a pointer to mutable data to a pointer
> to immutable data, so let’s fix all users of this API.

[Severity: Medium]
Is this commit message accurate regarding fixing all users?

Although many files are updated, prominent drivers like at24.c and at25.c
(and others outside drivers/nvmem/) still appear to use the legacy
.reg_write callback with the mutable void * signature.

Could the commit message be clarified to state that it only updates a subset
of drivers?

> diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
> index 2490f44caa409..02adfd787de85 100644
> --- a/drivers/nvmem/bcm-ocotp.c
> +++ b/drivers/nvmem/bcm-ocotp.c
[ ... ]
> @@ -179,11 +179,11 @@ static int bcm_otpc_read(void *context, unsigned int offset, void *val,
>  	return 0;
>  }
>  
> -static int bcm_otpc_write(void *context, unsigned int offset, void *val,
> -	size_t bytes)
> +static int bcm_otpc_write(void *context, unsigned int offset, const void *val,
> +			  size_t bytes)
>  {
>  	struct otpc_priv *priv = context;
> -	u32 *buf = val;
> +	const u32 *buf = val;
>  	u32 bytes_written;

[Severity: High]
This is a pre-existing issue, but does bcm_otpc_write() have an out-of-bounds
read if bytes is not a multiple of otpc_row_size * word_size?

If otpc_row_size > 1 (like 2 for the v2 map) and the nvmem core passes
bytes = 4, the inner loop iterates twice:

    for (i = 0; i < priv->map->otpc_row_size; i++) {
        writel(*buf, priv->base + priv->map->data_w_offset[i]);
        buf++;
        bytes_written += sizeof(*buf);
    }

Does this cause buf to advance beyond the provided length and read
uninitialized kernel memory into the hardware?

[ ... ]
> diff --git a/drivers/nvmem/lpc18xx_eeprom.c b/drivers/nvmem/lpc18xx_eeprom.c
> index 504155e30bab2..78a35ef095fbd 100644
> --- a/drivers/nvmem/lpc18xx_eeprom.c
> +++ b/drivers/nvmem/lpc18xx_eeprom.c
[ ... ]
> @@ -109,7 +109,7 @@ static int lpc18xx_eeprom_gather_write(void *context, unsigned int reg,
>  	usleep_range(100, 200);
>  
>  	while (bytes) {
> -		writel(*(u32 *)val, eeprom->mem_base + offset);
> +		writel(*(const u32 *)val, eeprom->mem_base + offset);
>  		ret = lpc18xx_eeprom_busywait_until_prog(eeprom);

[Severity: High]
This is a pre-existing issue, but can this loop run infinitely if bytes is
not a multiple of 4?

The loop decrements the unsigned bytes variable unconditionally by 4:

    bytes -= eeprom->val_bytes;

If the nvmem core passes an unaligned bytes value, does this subtraction
underflow and cause the loop to run indefinitely, leading to out-of-bounds
memory access? It appears lpc18xx_eeprom_read() shares this behavior.

[ ... ]
> diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
> index 1a7e4e5d8b86c..f960fa8cb25cf 100644
> --- a/drivers/nvmem/sprd-efuse.c
> +++ b/drivers/nvmem/sprd-efuse.c
> @@ -192,7 +192,7 @@ static void sprd_efuse_set_prog_en(struct sprd_efuse *efuse, bool en)
>  }
>  
>  static int sprd_efuse_raw_prog(struct sprd_efuse *efuse, u32 blk, bool doub,
> -			       bool lock, u32 *data)
> +			       bool lock, const u32 *data)
>  {

[Severity: Critical]
This is a pre-existing issue, but does sprd_efuse_raw_prog() read out of
bounds for small writes?

By dereferencing data as a 32-bit integer:

    writel(*data, efuse->base + SPRD_EFUSE_MEM(blk));

Could this cause an out-of-bounds read if the incoming buffer is less than
4 bytes long?

[ ... ]
> @@ -321,7 +321,7 @@ static int sprd_efuse_read(void *context, u32 offset, void *val, size_t bytes)
>  	return ret;
>  }
>  
> -static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
> +static int sprd_efuse_write(void *context, u32 offset, const void *val, size_t bytes)
>  {

[Severity: Critical]
This is a pre-existing issue, but is there a stack memory leak in
sprd_efuse_read()?

When the nvmem core passes a length up to PAGE_SIZE (4096) for sysfs reads,
sprd_efuse_read() allocates a local 4-byte data variable on the stack:

    u32 data;
    ...
    memcpy(val, &data, bytes);

If bytes > 4, would this copy up to 4092 bytes of stack memory to the
output buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726131724.15299-1-linkmauve@linkmauve.fr?part=3

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic