Re: [PATCH v2] mtd: virt-concat: Tear the concat down when its master goes away

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mtd: virt-concat: use-after-free in mtd_virt_concat_destroy() on 2nd unbind
- [Medium] mtd: virt-concat: partition leak in mtd_virt_concat_destroy_joins()
--

commit 9b28247f2646ce546494e78445d5bc76ddee956a
Author: Yuqiang Sun <izaiahsun@gmail.com>

mtd: virt-concat: Tear the concat down when its master goes away

This patch updates mtd_virt_concat_destroy() to tear down concatenated MTD
devices by matching the master of their component partitions, keeping each
partition in the slot of its DT node so the concatenation can be re-created
if a master is rebound. It also registers the mtd_info returned by
mtd_concat_create() directly and reference-counts the concatenated device via
struct mtd_virt_concat_dev so departing partitions are freed when the last
user closes the device.

> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> index 40b300021ddaa..64b0dd434ee24 100644
> --- a/drivers/mtd/mtd_virt_concat.c
> +++ b/drivers/mtd/mtd_virt_concat.c
[ ... ]
> +static struct mtd_virt_concat_dev *
> +mtd_virt_concat_unregister(struct mtd_virt_concat_node *item)
> +{
> +	struct mtd_virt_concat_dev *vdev = item->vdev;
> +
> +	if (vdev) {
> +		item->vdev = NULL;
> +		del_mtd_device(vdev->mtd);
> +	}
> +	return vdev;
>  }
>  
>  void mtd_virt_concat_destroy_joins(void)
>  {
> -	struct mtd_virt_concat_node *item, *tmp;
> -	struct mtd_info *mtd;
> +	struct mtd_virt_concat_node *item;
> +	struct mtd_virt_concat_dev *vdev;
>  
> -	list_for_each_entry_safe(item, tmp, &concat_node_list, head) {
> -		mtd = &item->concat->mtd;
> -		if (item->concat) {
> -			mtd_device_unregister(mtd);
> -			kfree(mtd->name);
> -			mtd_virt_concat_put_mtd_devices(item->concat);
> -			mtd_concat_destroy(mtd);
> -		}
> +	list_for_each_entry(item, &concat_node_list, head) {
> +		vdev = mtd_virt_concat_unregister(item);
> +		if (vdev)
> +			kref_put(&vdev->refs, mtd_virt_concat_release);
> +		kfree(item->concat);
> +		item->concat = NULL;
>  	}
>  }

[Severity: Medium]
Does mtd_virt_concat_destroy_joins() leak the struct mtd_info partition
objects and their name strings still stored in item->concat->subdev[]?

In mtd_device_parse_register(), add_mtd_partitions() passes concat partitions
to mtd_virt_concat_add(), which stores them in item->concat->subdev[idx]. If
a subsequent partition in add_mtd_partitions() fails or if
mtd_virt_concat_create_join() fails, the error path in
mtd_device_parse_register() calls del_mtd_device(mtd) directly without
calling mtd_virt_concat_destroy(mtd):

drivers/mtd/mtdcore.c:mtd_device_parse_register() {
	...
	if (IS_REACHABLE(CONFIG_MTD_VIRT_CONCAT)) {
		ret = mtd_virt_concat_create_join();
		if (ret < 0)
			goto out;
	}
	...
out:
	...
	if (ret && device_is_registered(&mtd->dev)) {
		err = del_mtd_device(mtd);
		if (err)
			pr_err("Error when deleting MTD device (%d)\n", err);
	}
}

For drivers that do not call mtd_device_unregister() when
mtd_device_parse_register() fails (such as spi_nor_probe() or
brcmnand_init_cs()), those partitions remain in item->concat->subdev[idx].
When cleanup_mtd() later calls mtd_virt_concat_destroy_joins() on module
unload, kfree(item->concat) frees the container without freeing the
partitions in item->concat->subdev[], and mtd_virt_concat_release() only
frees partitions that were moved into vdev->gone[].

>  /**
> - * mtd_virt_concat_destroy - Destroy the concat that includes the mtd object
> + * mtd_virt_concat_destroy - Destroy the concats that include partitions of
> + *                           an MTD device
>   * @mtd:        pointer to 'mtd_info'
>   *
> - * Return: 0 on success, -error otherwise.
> + * Called when the master of @mtd is unregistered. Partitions of that master
> + * that are part of a concatenation were never registered on their own, so
> + * unregister every concat that includes one of them, and free them once that
> + * concat is no longer in use. The other parts stay with the concat item,
> + * which is joined again once all its parts are back.
> + *
> + * Return: 0.
>   */
>  int mtd_virt_concat_destroy(struct mtd_info *mtd)
>  {
> -	struct mtd_info *child, *master = mtd_get_master(mtd);
> -	struct mtd_virt_concat_node *item, *tmp;
> +	struct mtd_info *part, *master = mtd_get_master(mtd);
> +	struct mtd_virt_concat_node *item;
> +	struct mtd_virt_concat_dev *vdev;
>  	struct mtd_concat *concat;
> -	int idx, ret = 0;
> -	bool is_mtd_found;
> -
> -	list_for_each_entry_safe(item, tmp, &concat_node_list, head) {
> -		is_mtd_found = false;
> +	int idx;
>  
> -		/* Find the concat item that hold the mtd device */
> -		for (idx = 0; idx < item->count; idx++) {
> -			if (item->nodes[idx] == mtd->dev.of_node) {
> -				is_mtd_found = true;
> -				break;
> -			}
> -		}
> -		if (!is_mtd_found)
> -			continue;
> +	list_for_each_entry(item, &concat_node_list, head) {
>  		concat = item->concat;
> +		vdev = NULL;
>  
> -		/*
> -		 * Since this concatenated device is being removed, retrieve
> -		 * all MTD devices that are part of it and register them
> -		 * individually.
> -		 */
> -		for (idx = 0; idx < concat->num_subdev; idx++) {
> -			child = concat->subdev[idx];
> -			if (child->dev.of_node != mtd->dev.of_node) {
> -				ret = add_mtd_device(child);
> -				if (ret)
> -					goto out;
> -			}
> -		}
> -		/* Destroy the concat */
> -		if (concat->mtd.name) {
> -			del_mtd_device(&concat->mtd);
> -			kfree(concat->mtd.name);
> -			mtd_virt_concat_put_mtd_devices(item->concat);
> -			mtd_concat_destroy(&concat->mtd);
> +		for (idx = 0; idx < item->count; idx++) {
> +			part = concat->subdev[idx];
> +			if (!part || mtd_get_master(part) != master)
> +				continue;
> +
> +			if (!vdev)
> +				vdev = mtd_virt_concat_unregister(item);
> +			concat->subdev[idx] = NULL;
> +			concat->num_subdev--;
> +			if (vdev)
> +				vdev->gone[vdev->num_gone++] = part;
> +			else
> +				mtd_virt_concat_free_part(part);
>  		}
> -
> -		list_del(&item->head);
> -		for (idx = 0; idx < item->count; idx++)
> -			of_node_put(item->nodes[idx]);
> -
> -		kfree(item);
> +		if (vdev)
> +			kref_put(&vdev->refs, mtd_virt_concat_release);
>  	}
> -	return 0;
> -out:
> -	mutex_lock(&master->master.partitions_lock);
> -	list_del(&child->part.node);
> -	mutex_unlock(&master->master.partitions_lock);
> -	kfree(mtd->name);
> -	kfree(mtd);
>  
> -	return ret;
> +	return 0;
>  }

[Severity: Critical]
As reported by Sashiko on v1 [1], can mtd_virt_concat_destroy() free a
partition while an open concatenated device still holds a pointer to it in
CONCAT(vdev->mtd)->subdev[]?

Consider a concatenated device vdev1 joining part0 on master0 and part1 on
master1 while a user holds vdev1->mtd open (so vdev1->refs is 2).

When master0 is unregistered first, mtd_virt_concat_destroy(master0) calls
mtd_virt_concat_unregister(item), which clears item->vdev = NULL and unregisters
vdev1->mtd. It places part0 into vdev1->gone[0] and drops the registration
reference on vdev1->refs from 2 to 1. Because vdev1->mtd is still open, vdev1
remains alive and CONCAT(vdev1->mtd)->subdev[1] still points to part1.

If master1 is then unregistered while vdev1->mtd is still held open,
mtd_virt_concat_destroy(master1) calls mtd_virt_concat_unregister(item), which
sees item->vdev == NULL and returns NULL. As a result,
mtd_virt_concat_destroy(master1) takes the else branch and immediately calls
mtd_virt_concat_free_part(part1).

Similarly, if master0 is bound again while vdev1->mtd is still open,
mtd_virt_concat_create_join() creates a new vdev2 that shares part1 in
concat->subdev[1] with vdev1 without per-partition reference counting. If
master1 is then unregistered while vdev2 is not open, part1 is placed into
vdev2->gone[] and immediately freed by mtd_virt_concat_release() when vdev2 is
put.

In either case, when the user later accesses or closes vdev1->mtd (for
example, mtdchar_close() -> mtd_sync() -> concat_sync() -> mtd_sync(subdev[1])),
won't it dereference the freed part1 structure?

[1] https://lore.kernel.org/r/sashiko-outbox-193135@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103080327.90193-1-izaiahsun@gmail.com?part=1

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