Re: [PATCH] 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 3 potential issue(s) to consider:
- [Critical] mtd: virt-concat: partition order corrupted on rebind after partial teardown
- [Critical] mtd: virt-concat: use-after-free when master unbinds with concat held open
- [High] mtd: virt-concat: double free of eraseregions after add_mtd_device() failure
--

commit 99c408477d51adb0dd45c7223ca8da76b077e7cf
Author: Yuqiang Sun <izaiahsun@gmail.com>

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

This commit updates mtd_virt_concat_destroy() to find and unregister virtual
concatenated MTD devices by matching the master of their component partitions,
freeing the departing master's partitions while retaining the concat item so
the device can be recreated if the master is bound again.

> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> index 40b300021ddaa..ecd5b07d32ce3 100644
> --- a/drivers/mtd/mtd_virt_concat.c
> +++ b/drivers/mtd/mtd_virt_concat.c
> @@ -57,94 +57,76 @@ static bool mtd_is_part_concat(struct device_node *dev)
>  	return false;
>  }
>  
> -static void mtd_virt_concat_put_mtd_devices(struct mtd_concat *concat)
> +/* Unregister the concatenated device, if it is registered */
> +static void mtd_virt_concat_unregister(struct mtd_concat *concat)
>  {
> -	int i;
> -
> -	for (i = 0; i < concat->num_subdev; i++)
> -		put_mtd_device(concat->subdev[i]);
> +	if (!concat->mtd.name)
> +		return;
> +
> +	del_mtd_device(&concat->mtd);
> +	kfree(concat->mtd.name);
> +	concat->mtd.name = NULL;
> +	if (concat->mtd.numeraseregions)
> +		kfree(concat->mtd.eraseregions);
> +	concat->mtd.numeraseregions = 0;
>  }

[Severity: High]
Can this double-free concat->mtd.eraseregions if add_mtd_device() fails in
mtd_virt_concat_create_join()?

In drivers/mtd/mtd_virt_concat.c:mtd_virt_concat_create_join(), *mtd is
copied into item->concat->mtd before add_mtd_device(&concat->mtd) is called:

	mtd = mtd_concat_create(concat->subdev, concat->num_subdev, name);
	if (!mtd) {
		kfree(name);
		return -ENXIO;
	}
	concat->mtd = *mtd;
	concat->mtd.dev.parent = concat->subdev[0]->dev.parent;
	concat->mtd.dev = concat->subdev[0]->dev;

	ret = add_mtd_device(&concat->mtd);
	if (ret)
		goto destroy_concat;
	...
destroy_concat:
	mtd_concat_destroy(mtd);

When mtd_concat_create() allocates mtd->eraseregions (when subdevices have
multiple erase regions), a failure in add_mtd_device() jumps to
destroy_concat and calls mtd_concat_destroy(mtd), which frees
mtd->eraseregions, but leaves concat->mtd.name non-NULL and
concat->mtd.numeraseregions > 0 with concat->mtd.eraseregions pointing to the
freed buffer.

When mtd_virt_concat_destroy() or mtd_virt_concat_destroy_joins() later calls
mtd_virt_concat_unregister(concat), it sees concat->mtd.name != NULL, calls
del_mtd_device(&concat->mtd) on the unregistered device, and calls
kfree(concat->mtd.eraseregions) a second time.

>  
>  void mtd_virt_concat_destroy_joins(void)
>  {
> -	struct mtd_virt_concat_node *item, *tmp;
> -	struct mtd_info *mtd;
> +	struct mtd_virt_concat_node *item;
>  
> -	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) {
> +		mtd_virt_concat_unregister(item->concat);
> +		mtd_concat_destroy(&item->concat->mtd);
> +		item->concat = NULL;
>  	}
>  }
>  
>  /**
> - * 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. 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_virt_concat_node *item;
>  	struct mtd_concat *concat;
> -	int idx, ret = 0;
> -	bool is_mtd_found;
> +	int idx, n;
>  
> -	list_for_each_entry_safe(item, tmp, &concat_node_list, head) {
> -		is_mtd_found = false;
> +	list_for_each_entry(item, &concat_node_list, head) {
> +		concat = item->concat;
>  
> -		/* 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;
> +		for (idx = 0; idx < concat->num_subdev; idx++) {
> +			if (mtd_get_master(concat->subdev[idx]) == master)
>  				break;
> -			}
>  		}
> -		if (!is_mtd_found)
> +		if (idx == concat->num_subdev)
>  			continue;
> -		concat = item->concat;
>  
> -		/*
> -		 * 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++) {
> +		mtd_virt_concat_unregister(concat);
> +
> +		for (idx = 0, n = 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;
> +			if (mtd_get_master(child) == master) {
> +				kfree(child->name);
> +				kfree(child);
> +			} else {
> +				concat->subdev[n++] = child;
>  			}
>  		}
> -		/* 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);
> -		}
> -
> -		list_del(&item->head);
> -		for (idx = 0; idx < item->count; idx++)
> -			of_node_put(item->nodes[idx]);
> -
> -		kfree(item);
> +		concat->num_subdev = n;

[Severity: Critical]
What happens if the concatenated MTD device is held open when its master is
unregistered?

Because concat->mtd.parent is NULL, opening the concatenated device increments
concat->mtd.refcnt (via __get_mtd_device()) without taking a reference on the
underlying subdevices or their masters.

When the master is unregistered while concat->mtd is open, del_mtd_device() in
mtd_virt_concat_unregister() only decrements concat->mtd.refcnt from 2 to 1
without running mtd_device_release(), so &concat->mtd remains live and in
mtd_idr.

However, mtd_virt_concat_unregister() immediately frees concat->mtd.name (and
sets it to NULL) and frees concat->mtd.eraseregions, and
mtd_virt_concat_destroy() immediately frees the child partition structs in
concat->subdev[] and shrinks concat->num_subdev to n without clearing trailing
entries in concat->subdev[].

While &concat->mtd remains open:
- reading /proc/mtd or /sys/class/mtd/mtdX/name dereferences NULL
  concat->mtd.name;
- concat_block_isbad() unconditionally dereferences concat->subdev[0] before
  checking concat->num_subdev, reading the freed child when n == 0;
- concat_erase() accesses freed concat->mtd.eraseregions, freed child/master
  structs, or hits BUG_ON(i >= concat->num_subdev);
- closing the open descriptor runs mtd_device_release() ->
  device_unregister(&concat->mtd.dev), which accesses concat->mtd.dev.parent
  (&master->dev) after master has already been freed.

[Severity: Critical]
Also, if a concatenation spans two masters (for example, item->nodes[0] on
master A and item->nodes[1] on master B), won't unbinding and rebinding master
A reverse the partition order in concat->subdev[]?

When master A is unregistered, mtd_virt_concat_destroy() frees master A's
partition at concat->subdev[0] and compacts master B's partition into
concat->subdev[0] via concat->subdev[n++] = child, setting
concat->num_subdev = 1.

When master A is bound again, drivers/mtd/mtd_virt_concat.c:mtd_virt_concat_add()
appends master A's partition at the end of concat->subdev[]:

	for (idx = 0; idx < item->count; idx++) {
		if (item->nodes[idx] == mtd->dev.of_node) {
			concat->subdev[concat->num_subdev++] = mtd;
			return true;
		}
	}

This places master A's partition at concat->subdev[1] instead of index 0, so
mtd_virt_concat_create_join() recreates the concatenated MTD device with its
partitions, flash offset mapping, device name, and parent device in reversed
order.

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

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