Hi, replying to sashiko
>commit ee93b9620650bb02f494ab88ef4e18464c95770f >Author: Gregor Herburger <[email protected]> >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.
