Re: [PATCH v4 2/5] nvmem: Add the Raspberry Pi OTP driver

On Fri, May 08, 2026 at 09:45:22PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] The driver exposes sensitive OTP memory (including device-specific private keys) with world-readable sysfs permissions.
> - [High] Unchecked firmware response size modifying `fwbuf->count` under `__counted_by_le()` annotation triggers a fortified memcpy panic.
> --
> 
> commit a8dbaf078fc585c391a3307f1adbf25d7433260e
> Author: Gregor Herburger <[email protected]>
> 
> 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..393640cb9e32b
> > --- /dev/null
> > +++ b/drivers/nvmem/raspberrypi-otp.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> [ ... ]
> > +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)
> > +{
> [ ... ]
> > +	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);
> 
> If the firmware performs a short read or encounters a partial error, does it
> update fwbuf->count to the actual number of words read?
> 
> If fwbuf->count is updated to a smaller value, the __counted_by_le() attribute
> will dynamically shrink the bounded size of fwbuf->data. Could this cause the
> memcpy() to trigger a fortified memcpy panic, since it uses the original
> requested bytes size?
> 

The firmware does not update the count field [1]. There is an extra field in the
tag header req_resp_size which is used by the firmware to indicate the length of
the response. If the response is longer the it gets truncated to fit into the
provided buffer. So I see no need to change something here.

> [ ... ]
> > +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,
> > +	};
> 
> The commit message notes this driver supports the "device specific private key
> OTP (BCM2712 only)".
> 
> Does the nvmem subsystem default to creating world-readable sysfs attributes
> unless explicitly restricted? Could this expose hardware private keys to
> unprivileged users via sysfs?
> 
> Should the config set .root_only = true to prevent this?

That's a valid point but I am not quite sure about this. From a hardware
perspective it seems the two otp regions are identical. That the 'private-otp'
is used for a private key seems to be purely conventional.

Also looking at the other nvmem drivers that set this to true (nintendo-otp,
rcare-efuse, qoriq-efuse and apple-efuses) seem to be drivers that all set the
read_only and seem to be drivers reading some preseeded keys or hardware
information.

I tend to set it to true for the bcm2712 private-otp and leave it false for the
customer-otp but would like to hear some more opinions on that.


Regards Gregor

[1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface