Re: [PATCH v5 4/8] nvmem: Add the Raspberry Pi OTP driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing Validation / FORTIFY_SOURCE Panic
- [High] Logic Error / Silent Data Loss on Read-Only OTPs
--

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

nvmem: Add the Raspberry Pi OTP driver

This patch introduces a new nvmem driver for accessing Raspberry Pi OTP
registers. The registers are accessed via the videocore firmware interface,
supporting both the customer OTP and the device specific private key OTP.

> diff --git a/drivers/nvmem/raspberrypi-otp.c b/drivers/nvmem/raspberrypi-otp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/nvmem/raspberrypi-otp.c

[ ... ]

> +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, bytes);

[Severity: High]
If the firmware returns a partial read and reduces the value of fwbuf->count,
will this memcpy trigger a FORTIFY_SOURCE bounds violation?

The rpi_otp_header struct uses __counted_by_le(count) for the data array. If
the firmware alters the count during rpi_firmware_property, the dynamically
evaluated object size of fwbuf->data will shrink.

Continuing to copy the originally requested bytes could exceed the updated
bounds and crash the kernel. Should the driver limit the copy size to the
returned count or handle short reads as an error?

[ ... ]

> +static int rpi_otp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_device *nvmem;
> +	struct rpi_otp_priv *priv;
> +	const struct rpi_otp_driver_data *data;
> +	struct nvmem_config config = {
> +		.read_only = false,
> +		.word_size = 4,
> +		.stride = 4,
> +		.reg_read = rpi_otp_read,
> +		.reg_write = rpi_otp_write,
> +		.id = NVMEM_DEVID_NONE,
> +	};

[Severity: High]
If the platform data specifies a read-only OTP region (like the device
specific private key OTP) with a write_tag of 0, could this cause silent
data loss during a write attempt?

If a write is attempted, rpi_otp_write would pass a tag of 0 to
rpi_firmware_property. Since tag 0 signifies RPI_FIRMWARE_PROPERTY_END,
the firmware will immediately terminate property processing, ignore the
payload, and return success. 

This would mislead the NVMEM subsystem and userspace into thinking the write
succeeded when the data was actually discarded. Could the driver conditionally
set .read_only = true and omit .reg_write if data->write_tag is 0?

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