[PATCH v3 0/4] nvmem: Add Raspberry Pi OTP nvmem driver

[Gregor Herburger]

Hi,

replying to sashiko

>commit ee93b9620650bb02f494ab88ef4e18464c95770f
>Author: Gregor Herburger <gregor.herburger at linutronix.de>
>nvmem: Add the Raspberry Pi OTP driver
>This commit adds a new nvmem driver to support OTP registers on Raspberry Pi
>platforms. It accesses the OTP memory through the videocore firmware
>interface.
>> diff --git a/drivers/nvmem/raspberrypi-otp.c b/drivers/nvmem/raspberrypi-otp.c
>> new file mode 100644
>> index 0000000000000..1edf2d5538d04
>> --- /dev/null
>> +++ b/drivers/nvmem/raspberrypi-otp.c
>> @@ -0,0 +1,120 @@
>[ ... ]
>> +struct rpi_otp_priv {
>> +	struct rpi_firmware *fw;
>> +	struct device *dev;
>> +	u32 read_tag;
>> +	u32 write_tag;
>> +};
>> +
>> +struct rpi_otp_header {
>> +	__le32 start;
>> +	__le32 count;
>> +	__le32 data[] __counted_by(count);
>Is the __counted_by attribute safe to use with explicitly endian types?
>On big-endian architectures (which are supported via COMPILE_TEST), the
>native bounds checking logic will evaluate the byte-swapped __le32 value as a
>massive integer (e.g., a count of 1 evaluates to 16777216).
>Does this unintentionally bypass the bounds checking defenses?

Yes that is right on compile tests on big-endian it will bypass the run time
check. I can use the __counted_by_le() macro to disable it on big-endian and
keep it on little-endian.

>> +};
>> +
>> +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 = bytes / 4;
>> +	int ret;
>> +
>> +	fwbuf = kzalloc(struct_size(fwbuf, data, count), GFP_KERNEL);
>Could an unaligned length request cause an out-of-bounds write here?
>If a caller requests an unaligned length (e.g., bytes = 3), count truncates
>to 0, allocating an 8-byte structure.

I first thought that alignment is checked in the nvmem system (with
nvmem_device::stride and nvmem_device::word_size)  but that is only true for
sysfs accesses. The nvmem_device_read/nvmem_device_write functions call directly
into the callback function of the driver. I guess there should be an alignment
check in this functions as well.

For now I will add an alignment check into my functions.

>> +	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);
>With an unaligned bytes request, would this memcpy write past the bounds of
>the 0-count fwbuf array? 
>Should there be an explicit alignment check for bytes and offset at the start
>of the function?

Yes see above.

>> +
>> +out:
>> +	kfree(fwbuf);
>> +	return ret;
>> +}
>> +
>> +static int rpi_otp_write(void *context, unsigned int offset, void *val, size_t bytes)
>> +{
>> +	struct rpi_otp_priv *priv = context;
>> +	struct rpi_otp_header *fwbuf;
>> +	u32 count = bytes / 4;
>> +	int ret;
>> +
>> +	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);
>> +	memcpy(fwbuf->data, val, bytes);
>Similarly, could this memcpy write out-of-bounds if bytes is not aligned to
>a 4-byte boundary?

Yes see above.

>> +
>> +	ret = rpi_firmware_property(priv->fw, priv->write_tag, fwbuf,
>> +				    sizeof(struct rpi_otp_header) + bytes);
>> +
>> +	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,
>> +		.word_size = 4,
>> +		.stride = 4,
>> +		.reg_read = rpi_otp_read,
>> +		.reg_write = rpi_otp_write,
>> +		.id = NVMEM_DEVID_NONE,
>> +	};
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	data = dev_get_platdata(dev);
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	priv->fw = dev_get_drvdata(dev->parent);
>> +	priv->dev = dev;
>> +	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;
>> +
>> +	nvmem = devm_nvmem_register(dev, &config);
>Does this devres usage introduce a use-after-free risk during device unbind?
>If an in-kernel consumer holds a reference to the nvmem device,
>devm_nvmem_unregister() drops its refcount but returns while the consumer's
>reference keeps the device alive.
>The devres framework will then proceed to free the priv structure.
>If the consumer subsequently calls nvmem_device_read(), will it dereference
>the freed priv pointer inside rpi_otp_read()?

Wow good catch. This took me a while to understand but i guess it can indeed
cause an use-after-free error error for in-kernel consumers. I will look into
it.