Re: [PATCH v3 4/7] block: implement NVMEM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Provider private data `bnv` is freed prematurely in `blk_nvmem_unregister`, leading to a Use-After-Free if consumers still hold an NVMEM cell reference.
- [High] Block device size check against `UINT_MAX` causes a sign-extension bug because `config.size` is a signed `int`.
- [Medium] Re-opening the block device by `devt` on every read allows NVMEM consumers to read from the wrong device if the original block device is hot-unplugged and its `devt` is reused.
--

commit 4b9cd41ee844820238b2e460a15697cc94e57c3f
Author: Daniel Golle <daniel@makrotopia.org>

block: implement NVMEM provider

This commit adds an NVMEM provider for block devices, allowing partitions
such as eMMC hardware partitions to expose MAC addresses and calibration
data to the NVMEM subsystem. It also adds an accessor to the core subsystem
to retrieve the underlying device.

> To safely defer the freeing of the provider private data until all
> consumers have released their cells, a nvmem_dev() accessor is added to
> the NVMEM core to expose the struct device embedded in struct nvmem_device.
> This allows registering a devm action on the nvmem device itself, ensuring
> the private data outlives any active cell references.

The commit message mentions registering a devm action to defer freeing
the private data. Does the code actually implement this action?

Looking at blk_nvmem_register(), there does not appear to be any devm
action registered to ensure the private data outlives active cell references.

> diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> new file mode 100644
> index 0000000000000..99c7728fb7bcc
> --- /dev/null
> +++ b/block/blk-nvmem.c
> @@ -0,0 +1,171 @@

[ ... ]

> +static int blk_nvmem_reg_read(void *priv, unsigned int from,
> +			      void *val, size_t bytes)
> +{
> +	blk_mode_t mode = BLK_OPEN_READ | BLK_OPEN_RESTRICT_WRITES;
> +	struct blk_nvmem *bnv = priv;
> +	size_t bytes_left = bytes;
> +	struct file *bdev_file;
> +	loff_t pos = from;
> +	int ret = 0;
> +
> +	bdev_file = bdev_file_open_by_dev(bnv->devt, mode, priv, NULL);

[Severity: Medium]
Could this open an unrelated block device if the original device is
hot-unplugged?

Since bdev_file_open_by_dev() looks up the current device associated with
bnv->devt, an unplug and subsequent reallocation of the same devt to a new
device might cause this read to access the wrong hardware.

blk_nvmem_unregister() does not mark the provider's bnv as dead or
invalidate the devt, which would allow a subsequent consumer read to
silently leak data from the newly plugged block device.

[ ... ]

> +static int blk_nvmem_register(struct device *dev)
> +{
> +	struct device_node *child, *np = dev_of_node(dev);
> +	struct block_device *bdev = dev_to_bdev(dev);
> +	struct nvmem_config config = {};
> +	struct blk_nvmem *bnv;
> +
> +	/* skip devices which do not have a device tree node */
> +	if (!np)
> +		return 0;
> +
> +	/* skip devices without an nvmem layout defined */
> +	child = of_get_child_by_name(np, "nvmem-layout");
> +	if (!child)
> +		return 0;
> +	of_node_put(child);
> +
> +	/*
> +	 * skip block device too large to be represented as NVMEM devices,
> +	 * the NVMEM reg_read callback uses an unsigned int offset
> +	 */
> +	if (bdev_nr_bytes(bdev) > UINT_MAX)
> +		return -EFBIG;

[Severity: High]
Does this check adequately protect config.size?

Because struct nvmem_config defines size as a signed int, a block device
capacity between 2GB and 4GB will pass this check but overflow into a
negative integer.

> +
> +	bnv = kzalloc_obj(*bnv);
> +	if (!bnv)
> +		return -ENOMEM;
> +
> +	config.id = NVMEM_DEVID_NONE;
> +	config.dev = &bdev->bd_device;
> +	config.name = dev_name(&bdev->bd_device);
> +	config.owner = THIS_MODULE;
> +	config.priv = bnv;
> +	config.reg_read = blk_nvmem_reg_read;
> +	config.size = bdev_nr_bytes(bdev);

When the nvmem core later assigns this to nvmem->size (which is size_t),
the negative integer will sign-extend to a large 64-bit value,
potentially breaking bounds checks.

[ ... ]

> +static void blk_nvmem_unregister(struct device *dev)
> +{
> +	struct blk_nvmem *bnv_c, *bnv_t, *bnv = NULL;
> +
> +	scoped_guard(mutex, &devices_mutex) {
> +		list_for_each_entry_safe(bnv_c, bnv_t, &nvmem_devices, list) {
> +			if (bnv_c->devt == dev_to_bdev(dev)->bd_device.devt) {
> +				bnv = bnv_c;
> +				list_del(&bnv->list);
> +				break;
> +			}
> +		}
> +
> +		if (!bnv)
> +			return;
> +	}
> +
> +	nvmem_unregister(bnv->nvmem);
> +	kfree(bnv);

[Severity: High]
Can this kfree() result in a use-after-free of bnv?

nvmem_unregister() only decrements the device reference count and
does not synchronously wait for consumers to release their cell references.

If a consumer subsequently reads from its cell, the core will invoke
blk_nvmem_reg_read() with the freed bnv pointer as the priv argument,
dereferencing bnv->devt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-block-as-nvmem-v3-0-82681f50aa35@oss.qualcomm.com?part=4