Re: [PATCH v6 04/10] nvmem: Add the Raspberry Pi OTP driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Potential FORTIFY_SOURCE out-of-bounds panic in `rpi_otp_read()` due to missing short-read validation.
- [High] `devm`-allocated private data is used-after-free if an external consumer holds a reference to the nvmem device after driver unbind.
- [Medium] Unconditional write support can lead to silent write failures on read-only OTP regions.
--

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

nvmem: Add the Raspberry Pi OTP driver

Raspberry Pis have OTP registers which can be accessed through the
videocore firmware. Add a nvmem driver to support these OTP registers.

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

[ ... ]

> +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]
Can this trigger a FORTIFY_SOURCE out-of-bounds panic?

The struct rpi_otp_header uses the __counted_by_le(count) attribute for the
flexible data array. If the firmware returns a short read or encounters an
error, it updates the count field to the actual number of words returned.

If count is reduced by the firmware, does this memcpy attempt to copy the
original requested size (bytes), exceeding the dynamically evaluated bounds
of fwbuf->data?

> +
> +out:
> +	kfree(fwbuf);
> +	return ret;
> +}

[ ... ]

> +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,

[Severity: Medium]
Will this allow silent write failures on read-only OTP regions?

If a region (like the device-specific private key OTP) is read-only and
lacks a valid write_tag in the platform data, write_tag will be 0. When
rpi_otp_write() sends tag 0 (which corresponds to RPI_FIRMWARE_PROPERTY_END)
to the firmware, it interprets this as an empty property list and returns
success without writing any data.

Should the read_only flag be set dynamically based on whether a valid
write_tag is provided?

> +		.word_size = 4,
> +		.stride = 4,
> +		.reg_read = rpi_otp_read,
> +		.reg_write = rpi_otp_write,
> +		.id = NVMEM_DEVID_NONE,
> +	};
> +
> +	data = dev_get_platdata(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->fw = dev_get_drvdata(dev->parent);
> +	priv->read_tag = data->read_tag;
> +	priv->write_tag = data->write_tag;
> +	config.dev = dev;
> +	config.priv = priv;
> +	config.name = data->name;
> +	config.size = data->size;
> +	config.root_only = data->root_only;
> +
> +	nvmem = devm_nvmem_register(dev, &config);

[Severity: High]
Could this lead to a use-after-free if an external consumer holds a
reference to the nvmem device after the driver unbinds?

The driver allocates the priv context using devm_kzalloc() and registers the
nvmem provider via devm_nvmem_register(). During driver unbind, devres cleans
up in reverse order: nvmem_unregister() is called, which drops the internal
kref, and then the priv memory is freed.

If an in-kernel consumer has acquired the nvmem device (e.g., via
nvmem_device_get()), the refcount remains greater than zero and the nvmem
device stays alive after priv is freed. If the consumer subsequently calls a
read or write operation, will rpi_otp_read() or rpi_otp_write() dereference
the freed priv pointer?

> +	if (IS_ERR(nvmem))
> +		return dev_err_probe(dev, PTR_ERR(nvmem), "error registering nvmem config\n");
> +
> +	return 0;
> +}

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