Re: [PATCH v9 05/12] nvmem: Add the Raspberry Pi OTP driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] nvmem: raspberrypi-otp: unvalidated fwbuf->count in
  rpi_otp_read()
--

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

nvmem: Add the Raspberry Pi OTP driver

This commit adds an nvmem driver to access Raspberry Pi OTP registers
through the VideoCore firmware.

> diff --git a/drivers/nvmem/raspberrypi-otp.c b/drivers/nvmem/raspberrypi-otp.c
> new file mode 100644
> index 0000000000000..2c632781f6003
> --- /dev/null
> +++ b/drivers/nvmem/raspberrypi-otp.c
> @@ -0,0 +1,131 @@

[ ... ]

> +struct rpi_otp_header {
> +	__le32 start;
> +	__le32 count;
> +	__le32 data[] __counted_by_le(count);
> +};
> +
> +static int rpi_otp_read(void *context, unsigned int offset, void *buf, size_t bytes)
> +{
> +	struct rpi_otp_priv *priv = context;
> +	struct rpi_otp_header *fwbuf;
> +	u32 count;
> +	int ret;
> +
> +	count = bytes / 4;
> +
> +	fwbuf = kzalloc(struct_size(fwbuf, data, count), GFP_KERNEL);
> +	if (!fwbuf)
> +		return -ENOMEM;
> +
> +	fwbuf->start = cpu_to_le32(offset / 4);
> +	fwbuf->count = cpu_to_le32(count);
> +
> +	ret = rpi_firmware_property(priv->fw, priv->read_tag, fwbuf,
> +				    sizeof(struct rpi_otp_header) + bytes);
> +	if (ret)
> +		goto out;
> +
> +	memcpy(buf, fwbuf->data, le32_to_cpu(fwbuf->count) * 4);

[Severity: High]
Can this overflow buf or leave part of it uninitialized if the firmware
modifies fwbuf->count?

rpi_firmware_property() overwrites fwbuf in-place with the response
payload, including fwbuf->count.  Because fwbuf->data is marked
__counted_by_le(count), updating fwbuf->count also changes the dynamic
object size seen by fortify checks.

If the firmware returns a count larger than the requested count (such as
0xffffffff on error), memcpy() reads past fwbuf->data and overflows the
bytes-sized destination buffer buf.  If it returns a smaller count,
memcpy() only partially fills buf while rpi_otp_read() still returns 0,
exposing uninitialized heap memory to callers such as bin_attr_nvmem_read().

Validating that le32_to_cpu(fwbuf->count) == count before copying (or
copying bytes after checking) would avoid both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-rpi-otp-driver-v9-0-0a7292a7a5b4@linutronix.de?part=5