Re: [PATCH 4/8] misc: make all reg_write callbacks take const void *

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

New issues:
- [Low] Passing `const void *` to `nvmem_device_write` discards the `const` qualifier, causing a compiler warning/error.

Pre-existing issues:
- [Critical] The `m24lr_write` function uses the device offset to index the source memory buffer, causing a critical out-of-bounds read of kernel memory.
- [High] Writing a dereferenced `const char *` byte to a 32-bit memory-mapped register triggers sign-extension, potentially writing corrupted values to the hardware.
- [High] Silent data loss when writing an odd number of bytes in 16-bit mode.
--

--- Patch [4]: [PATCH 4/8] misc: make all reg_write callbacks take const void * ---
commit 8fae9381bce13733c89619440fc083ff647125a4
Author: Link Mauve <[email protected]>

misc: 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 the nvmem_config API.

[ ... ]
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 5230e910a1d11..8de85433d593c 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -269,10 +269,10 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
>  }
>  
>  static int eeprom_93xx46_write(void *priv, unsigned int off,
> -				   void *val, size_t count)
> +			       const void *val, size_t count)
>  {
>  	struct eeprom_93xx46_dev *edev = priv;
> -	char *buf = val;
> +	const char *buf = val;

[Severity: High]
This is a pre-existing issue, but does this function silently drop data when
writing an odd number of bytes in 16-bit mode?

In 16-bit word mode (EE_ADDR16), the driver intercepts the call and truncates
the count:

    count &= ~1;

If count is 1, it becomes 0, skipping the write loop entirely and returning 0.
Because the nvmem core interprets a 0 return value as success, the caller
believes the data was written even though the hardware was unmodified.

[ ... ]
> diff --git a/drivers/misc/eeprom/m24lr.c b/drivers/misc/eeprom/m24lr.c
> index 7a9fd45a8e460..22f3221e232b5 100644
> --- a/drivers/misc/eeprom/m24lr.c
> +++ b/drivers/misc/eeprom/m24lr.c
> @@ -374,7 +374,7 @@ static int m24lr_nvmem_read(void *priv, unsigned int offset, void *val,
>  	return 0;
>  }
>  
> -static int m24lr_nvmem_write(void *priv, unsigned int offset, void *val,
> +static int m24lr_nvmem_write(void *priv, unsigned int offset, const void *val,
>  			     size_t bytes)

[Severity: Critical]
This is a pre-existing issue, but does m24lr_write() use the device offset to
index the source memory buffer?

When m24lr_nvmem_write() calls m24lr_write(..., val, bytes, offset, true),
m24lr_write() passes buf + offset to the regmap write instead of advancing
by the number of bytes written:

    err = m24lr_regmap_write(regmap, buf + offset, n, offset);

Since buf is only sized for the data chunk being written, adding the device
offset pushes the pointer far past the end of buf, leading to an out-of-bounds
read of kernel memory that gets written to the external EEPROM.

>  {
>  	ssize_t err;

[ ... ]
> diff --git a/drivers/misc/keba/cp500.c b/drivers/misc/keba/cp500.c
> index 6c65fbf22e756..ee7889cb63a8b 100644
> --- a/drivers/misc/keba/cp500.c
> +++ b/drivers/misc/keba/cp500.c
> @@ -590,7 +590,7 @@ static int cp500_nvmem_read(void *priv, unsigned int offset, void *val,
>  	return 0;
>  }
>  
> -static int cp500_nvmem_write(void *priv, unsigned int offset, void *val,
> +static int cp500_nvmem_write(void *priv, unsigned int offset, const void *val,
>  			     size_t bytes)
>  {
>  	struct cp500_nvmem *nvmem = priv;

[Severity: Low]
Does passing the now-const val directly to nvmem_device_write() discard the
const qualifier?

nvmem_device_write() expects a mutable void pointer, which will cause a build
warning (and potentially break the build if -Werror is enabled):

    ret = nvmem_device_write(nvmem->base_nvmem, nvmem->offset + offset,
                             bytes, val);

[ ... ]
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> index a2ed477e0370b..f2759e9b41386 100644
> --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> @@ -159,11 +159,11 @@ static int pci1xxxx_eeprom_read(void *priv_t, unsigned int off,
>  }
>  
>  static int pci1xxxx_eeprom_write(void *priv_t, unsigned int off,
> -				 void *value_t, size_t count)
> +				 const void *value_t, size_t count)
>  {
>  	struct pci1xxxx_otp_eeprom_device *priv = priv_t;
>  	void __iomem *rb = priv->reg_base;
> -	char *value = value_t;
> +	const char *value = value_t;

[Severity: High]
This is a pre-existing issue, but does dereferencing this const char pointer
trigger sign-extension when passed to writel()?

Later in this function, the byte is written like this:

    writel(*(value + byte), rb + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));

If the byte has its most significant bit set (e.g. is negative), the C compiler
will sign-extend it to a 32-bit negative integer before passing it to writel(),
which writes 1s into the upper 24 bits of the register. Should value be cast to
const u8 * to ensure safe zero-extension?

>  	u32 regval;
>  	u32 byte;
>  	int ret;
> @@ -262,11 +262,11 @@ static int pci1xxxx_otp_read(void *priv_t, unsigned int off,
>  }
>  
>  static int pci1xxxx_otp_write(void *priv_t, unsigned int off,
> -			      void *value_t, size_t count)
> +			      const void *value_t, size_t count)
>  {
>  	struct pci1xxxx_otp_eeprom_device *priv = priv_t;
>  	void __iomem *rb = priv->reg_base;
> -	char *value = value_t;
> +	const char *value = value_t;

[Severity: High]
This is a pre-existing issue, but this has the same sign-extension behavior
when writing to OTP_PRGM_DATA_OFFSET:

    writel(*(value + byte), rb + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));

Should this pointer also be changed to const u8 *?

>  	u32 regval;
>  	u32 byte;
>  	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic