Re: [PATCH v2 4/8] block: implement NVMEM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect page offset calculation and iteration logic for large folios causes severe data corruption.
- [High] Dereferencing NULL when reading from a block device page cache allocated in high memory.
- [High] Use-after-free of provider private data `bnv` on the `devm_add_action` error path.
- [Medium] Leaked device node reference in `blk_nvmem_register` when checking for the layout node.
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race allows cross-device arbitrary data reads due to `devt` reallocation.
--

commit a3bbd821536b45bed34dfa8815ddf9b55f9bb8b9
Author: Daniel Golle <daniel@makrotopia.org>
Subject: block: implement NVMEM provider

This commit adds an NVMEM provider for block devices, allowing partitions on
embedded devices to be accessed via the NVMEM layer. It introduces an
nvmem_dev() accessor to safely defer freeing provider private data by adding
a devm action to the nvmem device.

> diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> new file mode 100644
> index 0000000000000..96c0ffc51b186
> --- /dev/null
> +++ b/block/blk-nvmem.c
> @@ -0,0 +1,188 @@
[ ... ]
> +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;
> +	unsigned long offs = from & ~PAGE_MASK, to_read;
> +	pgoff_t f_index = from >> PAGE_SHIFT;
> +	struct blk_nvmem *bnv = priv;
> +	size_t bytes_left = bytes;
> +	struct file *bdev_file;
> +	struct folio *folio;
> +	void *p;
> +	int ret = 0;
> +
> +	if (bnv->removed)
> +		return -ENODEV;
> +
> +	bdev_file = bdev_file_open_by_dev(bnv->devt, mode, priv, NULL);

Is there a race condition between the removed check and opening the device?

The check for bnv->removed is performed without locks. If
blk_nvmem_unregister() runs concurrently and the original block device is
hot-removed, its devt identifier could be freed and reassigned to a newly
plugged-in, unrelated block device.

If this thread is preempted after the lockless check, it could open the new
block device using the recycled devt and read data from the wrong disk.

> +	if (!bdev_file)
> +		return -ENODEV;
> +
> +	if (IS_ERR(bdev_file))
> +		return PTR_ERR(bdev_file);
> +
> +	while (bytes_left) {
> +		folio = read_mapping_folio(bdev_file->f_mapping, f_index++, NULL);
> +		if (IS_ERR(folio)) {
> +			ret = PTR_ERR(folio);
> +			goto err_release_bdev;
> +		}
> +		to_read = min_t(unsigned long, bytes_left, PAGE_SIZE - offs);
> +		p = folio_address(folio) + offset_in_folio(folio, offs);

Does this logic handle large folios correctly?

If read_mapping_folio() returns a large folio, it will return the same head
folio for multiple sequential f_index values.

Because offs is repeatedly reset to 0 in subsequent loop iterations,
offset_in_folio(folio, 0) will evaluate to 0, causing p to repeatedly point
to the start of the head page instead of advancing through the subpages of
the large folio.

Additionally, can this cause a NULL pointer dereference on 32-bit platforms
with HIGHMEM?

Block device page cache mappings can allocate folios from the HIGHMEM zone,
and folio_address() relies on page_address(), which returns NULL for
unmapped high memory pages.

Adding an offset to this NULL pointer and attempting to read from it might
crash the kernel. Mapping the folio safely with kmap_local_folio() or
memcpy_from_folio() might be needed here.

> +		memcpy(val, p, to_read);
> +		offs = 0;
> +		bytes_left -= to_read;
> +		val += to_read;
> +		folio_put(folio);
> +	}
> +
> +err_release_bdev:
> +	fput(bdev_file);
> +
> +	return ret;
> +}
> +
> +static int blk_nvmem_register(struct device *dev)
> +{
> +	struct device_node *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 */
> +	if (!of_get_child_by_name(np, "nvmem-layout"))
> +		return 0;

Does this leak the device node reference? of_get_child_by_name() increments
the reference count of the returned child device node, but since the pointer
is not saved, of_node_put() can never be called to release it.

> +
> +	/*
> +	 * skip block device too large to be represented as NVMEM devices
> +	 * which are using an 'int' as address
> +	 */
> +	if (bdev_nr_bytes(bdev) > INT_MAX)
> +		return -EFBIG;
> +
> +	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);
> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.read_only = true;
> +	config.root_only = true;
> +	config.ignore_wp = true;
> +	config.of_node = to_of_node(dev->fwnode);
> +
> +	bnv->devt = bdev->bd_device.devt;
> +	bnv->nvmem = nvmem_register(&config);
> +	if (IS_ERR(bnv->nvmem)) {
> +		dev_err_probe(&bdev->bd_device, PTR_ERR(bnv->nvmem),
> +			      "Failed to register NVMEM device\n");
> +
> +		kfree(bnv);
> +		return PTR_ERR(bnv->nvmem);
> +	}
> +
> +	/*
> +	 * Free bnv only when the nvmem device is fully released (i.e. when
> +	 * its kref hits zero), not at unregister time. This prevents a
> +	 * use-after-free if a consumer still holds an nvmem_cell reference
> +	 * when the block device is removed: nvmem_unregister() only does a
> +	 * kref_put(), so reg_read could still be called with bnv as priv
> +	 * until the last consumer drops its cell.
> +	 */
> +	if (devm_add_action(nvmem_dev(bnv->nvmem), blk_nvmem_free, bnv)) {
> +		nvmem_unregister(bnv->nvmem);
> +		kfree(bnv);

Could this result in a use-after-free if a consumer acquires a reference
before devm_add_action() fails?

nvmem_register() makes the NVMEM device globally visible. If
nvmem_unregister() is called on the error path, it only drops the
provider's reference.

If a consumer has already obtained a reference, calling kfree(bnv)
immediately will free the provider data while the consumer might still
invoke blk_nvmem_reg_read().

> +		return -ENOMEM;
> +	}
> +
> +	mutex_lock(&devices_mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-block-as-nvmem-v2-0-bf17edd5134e@oss.qualcomm.com?part=4