Re: [PATCH v3 01/58] nvmem: disconnect provider callbacks before unregister returns

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] nvmem: nvmem_populate_sysfs_cells() skips layout cells and
  tags provider cells
- [Medium] nvmem: leaked devres and stale cells_group in
  nvmem_populate_sysfs_cells()
- [Medium] nvmem: unpaired NVMEM_REMOVE notifier event on
  nvmem_register() failure
- [Medium] nvmem: sysfs bin_attribute UAF during device_del() in
  nvmem_unregister()
- [Low] nvmem: unreachable disconnected check in nvmem_device_match()
--

commit 7e668831f84b8e65137e3150ab3a241d5d20c765
Author: James Hilliard <james.hilliard1@gmail.com>

nvmem: disconnect provider callbacks before unregister returns

This commit disconnects NVMEM provider and layout callbacks using SRCU
when unregistering a provider or layout, and adds reference counting to
struct nvmem_cell_entry so consumers can safely outlive provider unbind.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 0556d140170a4..8571d821591c9 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
[ ... ]
> @@ -511,9 +516,10 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
>  		i++;
>  	}
>  
> -	group.bin_attrs = pattrs;
> +	nvmem->cells_group.name = "cells";
> +	nvmem->cells_group.bin_attrs = pattrs;
>  
> -	ret = device_add_group(&nvmem->dev, &group);
> +	ret = device_add_group(&nvmem->dev, &nvmem->cells_group);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Does an error in nvmem_populate_sysfs_cells() during layout registration
leak the devres allocations on nvmem->dev and leave a stale
nvmem->cells_group.bin_attrs pointer?

pattrs, attrs, and attrs[i].attr.name are allocated with devm_kcalloc()
and devm_kasprintf() on the parent &nvmem->dev, not &layout->dev.  If one
of those allocations or device_add_group() fails (for example, with
-EEXIST on a duplicate cell attribute name), nvmem->sysfs_cells_populated
remains false.  When nvmem_layout_register() then unwinds through
nvmem_layout_unregister(), the devm_kfree() cleanup is skipped because
nvmem->sysfs_cells_populated is false, leaking those allocations across
layout probe retries and leaving nvmem->cells_group.bin_attrs pointing to
attributes whose private pointers reference freed nvmem_cell_entry
objects.

[ ... ]

> @@ -663,6 +682,8 @@ int nvmem_add_one_cell(struct nvmem_device *nvmem,
>  		return rval;
>  	}
>  
> +	kref_init(&cell->refcnt);
> +	cell->layout = nvmem->layout;
>  	nvmem_cell_entry_add(cell);
>  
>  	return 0;
[ ... ]
> @@ -851,29 +877,81 @@ int nvmem_layout_register(struct nvmem_layout *layout)
>  	if (!layout->add_cells)
>  		return -EINVAL;
>  
> -	/* Populate the cells */
> -	ret = layout->add_cells(layout);
> +	scoped_guard(srcu, &layout->nvmem->srcu) {
> +		if (READ_ONCE(layout->nvmem->disconnected))
> +			return -ENODEV;
> +		ret = layout->add_cells(layout);
> +	}
>  	if (ret)
> -		return ret;
> +		goto err_unregister;
>  
>  #ifdef CONFIG_NVMEM_SYSFS
>  	ret = nvmem_populate_sysfs_cells(layout->nvmem);
> -	if (ret) {
> -		nvmem_device_remove_all_cells(layout->nvmem);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_unregister;
>  #endif
>  
>  	return 0;
> +
> +err_unregister:
> +	nvmem_layout_unregister(layout);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(nvmem_layout_register);
>  
>  void nvmem_layout_unregister(struct nvmem_layout *layout)
>  {
> -	/* Keep the API even with an empty stub in case we need it later */
> +	struct nvmem_device *nvmem = layout->nvmem;
> +	struct nvmem_cell_entry *cell, *next;
> +	struct attribute_group group = {};
> +	LIST_HEAD(cells);
> +
> +	scoped_guard(mutex, &nvmem_mutex) {
> +		if (nvmem->sysfs_cells_populated)
> +			group = nvmem->cells_group;
> +		list_for_each_entry_safe(cell, next, &nvmem->cells, node) {
> +			if (cell->layout != layout)
> +				continue;
> +			WRITE_ONCE(cell->disconnected, true);
> +			list_move_tail(&cell->node, &cells);
> +		}
> +	}
> +
> +	if (group.bin_attrs) {
> +		device_remove_group(&nvmem->dev, &group);
> +		scoped_guard(mutex, &nvmem_mutex) {
> +			nvmem->sysfs_cells_populated = false;
> +			memset(&nvmem->cells_group, 0, sizeof(nvmem->cells_group));
> +		}
> +		for (int i = 0; group.bin_attrs[i]; i++)
> +			devm_kfree(&nvmem->dev, (void *)group.bin_attrs[i]->attr.name);
> +		devm_kfree(&nvmem->dev, (void *)group.bin_attrs[0]);
> +		devm_kfree(&nvmem->dev, group.bin_attrs);
> +	}
> +
> +	/* Old cell handles retain data, never a departing layout's callbacks. */
> +	synchronize_srcu(&nvmem->srcu);
> +	list_for_each_entry_safe(cell, next, &cells, node) {
> +		blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
> +		list_del(&cell->node);
> +		nvmem_cell_entry_put(cell);
> +	}
> +#ifdef CONFIG_NVMEM_SYSFS
> +	if (!READ_ONCE(nvmem->disconnected) && nvmem_populate_sysfs_cells(nvmem))
> +		dev_warn(&nvmem->dev, "Failed to restore provider cell attributes\n");
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(nvmem_layout_unregister);

[Severity: Medium]
Can nvmem_populate_sysfs_cells() skip layout cell sysfs attributes when
provider cell attributes are already populated, and can nvmem_add_one_cell()
misattribute provider cells to nvmem->layout?

If an nvmem_device has provider cells (or if nvmem_layout_unregister()
restored provider cell attributes on unbind), nvmem_populate_sysfs_cells()
has already set nvmem->sysfs_cells_populated = true.  When a layout driver
later probes (or rebinds) and nvmem_layout_register() calls
nvmem_populate_sysfs_cells(), it sees nvmem->sysfs_cells_populated == true
and returns 0 without rebuilding nvmem->cells_group, omitting the layout
cells from sysfs.  A similar race exists during nvmem_register() because
nvmem_populate_layout() publishes layout->dev on nvmem_layout_bus_type
before nvmem_register() calls nvmem_populate_sysfs_cells().

In addition, nvmem->layout is set in nvmem_layout_create_device() for the
lifetime of the layout device, so setting cell->layout = nvmem->layout in
nvmem_add_one_cell() tags any cells added by a provider after
nvmem_register() (such as in mtk_mfg_init_nvmem_provider() or
u_boot_env_probe()) as layout cells and removes them on layout unbind.

[ ... ]

> @@ -1010,7 +1086,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  
>  	rval = nvmem_populate_layout(nvmem);
>  	if (rval)
> -		goto err_remove_dev;
> +		goto err_unregister;
>  
>  	/* If the device has WP GPIO, default to read-only */
>  	if (nvmem->wp_gpio)
> @@ -1019,23 +1095,24 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  #ifdef CONFIG_NVMEM_SYSFS
>  	rval = nvmem_populate_sysfs_cells(nvmem);
>  	if (rval)
> -		goto err_destroy_layout;
> +		goto err_unregister;
>  #endif
>  
>  	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>  
>  	return nvmem;
>  
> -#ifdef CONFIG_NVMEM_SYSFS
> -err_destroy_layout:
> -	nvmem_destroy_layout(nvmem);
> -#endif
> -err_remove_dev:
> -	device_del(&nvmem->dev);
> +err_unregister:
> +	nvmem_unregister(nvmem);
> +	return ERR_PTR(rval);

[Severity: Medium]
Does jumping to err_unregister on failure in nvmem_populate_layout() or
nvmem_populate_sysfs_cells() emit an unpaired NVMEM_REMOVE notification?

blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem) is only
called after both functions succeed, but nvmem_unregister()
unconditionally sends NVMEM_REMOVE.

[ ... ]

> @@ -1048,24 +1125,29 @@ static void nvmem_device_release(struct kref *kref)
>  
>  	nvmem = container_of(kref, struct nvmem_device, refcnt);
>  
> -	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_REMOVE, nvmem);
> -
> -	nvmem_sysfs_remove_compat(nvmem);
> -
> -	nvmem_device_remove_all_cells(nvmem);
> -	nvmem_destroy_layout(nvmem);
> -	device_unregister(&nvmem->dev);
> +	put_device(&nvmem->dev);
>  }
>  
>  /**
>   * nvmem_unregister() - Unregister previously registered nvmem device
>   *
>   * @nvmem: Pointer to previously registered nvmem device.
> + *
> + * Stop and drain provider callbacks before returning. Existing consumers keep
> + * their core-owned metadata, but further access returns -ENODEV.
>   */
>  void nvmem_unregister(struct nvmem_device *nvmem)
>  {
> -	if (nvmem)
> -		kref_put(&nvmem->refcnt, nvmem_device_release);
> +	if (!nvmem)
> +		return;
> +
> +	nvmem_disconnect(nvmem);
> +	nvmem_destroy_layout(nvmem);
> +	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_REMOVE, nvmem);
> +	nvmem_sysfs_remove_compat(nvmem);
> +	device_del(&nvmem->dev);
> +	nvmem_device_remove_all_cells(nvmem);
> +	kref_put(&nvmem->refcnt, nvmem_device_release);
>  }

[Severity: Medium]
Can device_del(&nvmem->dev) in nvmem_unregister() cause a use-after-free
on the devres-allocated bin_attribute array if a sysfs read races with
unregistration?

nvmem_populate_sysfs_cells() allocates pattrs, attrs, and
attrs[i].attr.name with devm_kcalloc() and devm_kasprintf() on &nvmem->dev,
and registers them via non-devres device_add_group(&nvmem->dev,
&nvmem->cells_group).  When nvmem_unregister() runs with provider cell
attributes populated, it calls device_del(&nvmem->dev) without first
calling device_remove_group(&nvmem->dev, &nvmem->cells_group):

CPU0                                  CPU1
nvmem_unregister()
  device_del(&nvmem->dev)
    devres_release_all(&nvmem->dev)
      /* frees attrs */
                                      sysfs_kf_bin_read()
                                        battr = of->kn->priv;
                                        battr->read(...)  <-- use-after-free
    kobject_del(&nvmem->dev.kobj)

In device_del(), device_remove_attrs() only removes dev->groups, so
nvmem->cells_group remains active in sysfs until kobject_del() runs after
devres_release_all().

[ ... ]

> @@ -1103,26 +1185,46 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_nvmem_register);
>  
> +struct nvmem_match_data {
> +	void *data;
> +	int (*match)(struct device *dev, const void *data);
> +};
> +
> +static int nvmem_match_device(struct device *dev, const void *data)
> +{
> +	const struct nvmem_match_data *match = data;
> +	struct nvmem_device *nvmem = to_nvmem_device(dev);
> +
> +	guard(srcu)(&nvmem->srcu);
> +	if (READ_ONCE(nvmem->disconnected))
> +		return 0;
> +	return match->match(dev, match->data);
> +}
> +
>  static struct nvmem_device *nvmem_device_match(void *data,
>  			int (*match)(struct device *dev, const void *data))
>  {
> +	struct nvmem_match_data match_data = { .data = data, .match = match };
>  	struct nvmem_device *nvmem = NULL;
>  	struct device *dev;
>  
> -	scoped_guard(mutex, &nvmem_mutex) {
> -		dev = bus_find_device(&nvmem_bus_type, NULL, data, match);
> -		if (dev)
> -			nvmem = to_nvmem_device(dev);
> -	}
> +	guard(mutex)(&nvmem_mutex);
> +	dev = bus_find_device(&nvmem_bus_type, NULL, &match_data, nvmem_match_device);
> +	if (dev)
> +		nvmem = to_nvmem_device(dev);
>  	if (!nvmem)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> +	if (READ_ONCE(nvmem->disconnected)) {
> +		put_device(dev);
> +		return ERR_PTR(-ENODEV);
> +	}

[Severity: Low]
Is the second READ_ONCE(nvmem->disconnected) check in nvmem_device_match()
reachable?

nvmem_disconnect() sets nvmem->disconnected under nvmem_mutex, and
nvmem_device_match() holds nvmem_mutex across bus_find_device(), where
nvmem_match_device() already rejects disconnected devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-submit-rawnand-retained-handle-lifetime-v1-v3-0-ce537999741d@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/