Re: [PATCH v3] 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:
- [High] mtd: virt-concat: dangling subdev pointers on registration failure
- [High] mtd: virt-concat: unlocked concurrent access to concat_node_list and item state
- [High] mtd: virt-concat: open concat device lacks master ref and stays live on unbind
--

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

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

This commit updates virtual MTD concatenation teardown to match partitions
by their master device rather than DT node, registers the device returned
by mtd_concat_create() directly, and tracks references on both the
concatenated device and its subdevice partitions across master unbind and
re-bind cycles.

> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> index 40b300021ddaa..84d044bd00e96 100644
> --- a/drivers/mtd/mtd_virt_concat.c
> +++ b/drivers/mtd/mtd_virt_concat.c
[ ... ]
> +static int mtd_virt_concat_get_device(struct mtd_info *mtd)
> +{
> +	struct mtd_virt_concat_dev *vdev = mtd->priv;
>  
> -	for (i = 0; i < concat->num_subdev; i++)
> -		put_mtd_device(concat->subdev[i]);
> +	kref_get(&vdev->refs);
> +	return 0;
> +}
> +
> +static void mtd_virt_concat_put_device(struct mtd_info *mtd)
> +{
> +	struct mtd_virt_concat_dev *vdev = mtd->priv;
> +
> +	kref_put(&vdev->refs, mtd_virt_concat_release);
> +}
> +
> +/* Unregister the concatenated device of @item, if it is registered */
> +static void 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);
> +		kref_put(&vdev->refs, mtd_virt_concat_release);
> +	}
>  }

[Severity: High]
As reported by Sashiko on v1 [1], what happens when the concatenated MTD
device is open while one of its underlying flash masters is unbound?

CONFIG_MTD_VIRT_CONCAT depends on CONFIG_MTD_PARTITIONED_MASTER. In
mtd_virt_concat_create_join(), vdev->mtd is created by mtd_concat_create()
with vdev->mtd->parent == NULL, while mtd->dev = concat->subdev[0]->dev
sets vdev->mtd->dev.parent = &master0->dev and vdev->mtd->dev.of_node =
subdev[0]->dev.of_node.

When a user opens vdev->mtd, __get_mtd_device() calls
mtd_virt_concat_get_device() and increments vdev->mtd->refcnt from 1 to 2,
but because vdev->mtd->parent is NULL, it does not increment
master0->refcnt.

When a master is unbound while vdev->mtd is open,
mtd_virt_concat_unregister() calls del_mtd_device(vdev->mtd), which only
decrements vdev->mtd->refcnt from 2 to 1 without running
mtd_device_release(). That leaves vdev->mtd in mtd_idr and sysfs until it
is closed, and because mtd_virt_concat_get_device() unconditionally
increments vdev->refs and returns 0, new callers can still open the
unregistered vdev->mtd. If the unbound master is bound again while the old
vdev->mtd is still open, mtd_virt_concat_create_join() registers a new
vdev->mtd with the same name and subdev[0]->dev.of_node: if that DT node
has an mtd alias, add_mtd_device() fails idr_alloc() with -ENOSPC, and
otherwise get_mtd_device_nm() and of_get_mtd_device_by_node() return the
old unregistered vdev->mtd at the lower mtd_idr index.

In addition, when master0 (subdev[0]'s master) is unbound while vdev->mtd
is open, del_mtd_device(master0) drops master0->refcnt from 1 to 0 and
immediately invokes mtd_device_release(master0) while vdev->mtd->dev is
still registered with vdev->mtd->dev.parent == &master0->dev:

drivers/mtd/mtdcore.c:mtd_device_release() {
    ...
    device_unregister(&mtd->dev);

    idr_remove(&mtd_idr, mtd->index);
    of_node_put(mtd_get_of_node(mtd));

    /* Clear dev so mtd can be safely re-registered later if desired */
    memset(&mtd->dev, 0, sizeof(mtd->dev));
    ...
}

Because vdev->mtd->dev holds a parent kobject reference on master0->dev,
device_unregister(&master0->dev) only drops master0->dev.kobj.kref from 2
to 1 (so mtd_release() does not run), and memset(&master0->dev, 0, ...)
then zeroes master0->dev while vdev->mtd->dev still points to it, leading
to a kobject refcount underflow and use-after-free when vdev->mtd is
closed.

[ ... ]
>  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_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;
>  
> -		/*
> -		 * 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;
> +
> +			mtd_virt_concat_unregister(item);
> +			concat->subdev[idx] = NULL;
> +			concat->num_subdev--;
> +			mtd_virt_concat_put_part(part);
>  		}
> -
> -		list_del(&item->head);
> -		for (idx = 0; idx < item->count; idx++)
> -			of_node_put(item->nodes[idx]);
> -
> -		kfree(item);
>  	}
> -	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: High]
When a concatenated MTD device spans partitions across two separate master
devices, what serializes concurrent calls to mtd_virt_concat_add(),
mtd_virt_concat_create_join(), and mtd_virt_concat_destroy() across the
two independent master devices?

None of the callers in mtd_device_parse_register(), add_mtd_partitions(),
or mtd_device_unregister() hold mtd_table_mutex or another shared lock, and
there is no lock in drivers/mtd/mtd_virt_concat.c protecting
concat_node_list, item->vdev, concat->subdev[], or concat->num_subdev:

1. If CPU 0 runs mtd_virt_concat_create_join() when the second master
probes, sees item->count == concat->num_subdev and !item->vdev, and blocks
in kmalloc(), kzalloc_flex(), or mtd_concat_create() before incrementing
vdev->parts[idx]->refcnt and setting item->vdev = vdev, CPU 1 can
concurrently unbind the first master in mtd_virt_concat_destroy(). CPU 1
sees item->vdev == NULL in mtd_virt_concat_unregister(item), clears
concat->subdev[0], decrements concat->num_subdev, and calls
mtd_virt_concat_put_part(part), freeing part and part->name while CPU 0 is
still reading concat->subdev[0] in sprintf(), mtd_concat_create(), and
kref_get(&vdev->parts[0]->refcnt).

2. If both masters are unbound concurrently on two CPUs, both can enter
mtd_virt_concat_unregister(item) and read the same non-NULL vdev =
item->vdev before item->vdev = NULL is written, causing both CPUs to call
del_mtd_device(vdev->mtd) and kref_put(&vdev->refs,
mtd_virt_concat_release) and drop the registration reference on vdev->refs
twice.

3. Concurrent probes of two masters perform unsynchronized
concat->num_subdev++ updates in mtd_virt_concat_add() and can both observe
!item->vdev across the sleeping allocations in
mtd_virt_concat_create_join(), registering two concatenated devices and
overwriting item->vdev.

Should accesses to concat_node_list and item state be serialized with a
mutex?

> @@ -226,7 +271,11 @@ bool mtd_virt_concat_add(struct mtd_info *mtd)
>  		concat = item->concat;
>  		for (idx = 0; idx < item->count; idx++) {
>  			if (item->nodes[idx] == mtd->dev.of_node) {
> -				concat->subdev[concat->num_subdev++] = mtd;
> +				if (concat->subdev[idx])
> +					return false;
> +				kref_init(&mtd->refcnt);
> +				concat->subdev[idx] = mtd;
> +				concat->num_subdev++;
>  				return true;
>  			}
>  		}
> @@ -287,6 +336,7 @@ int mtd_virt_concat_node_create(void)
>  int mtd_virt_concat_create_join(void)
>  {
>  	struct mtd_virt_concat_node *item;
> +	struct mtd_virt_concat_dev *vdev;
>  	struct mtd_concat *concat;
>  	struct mtd_info *mtd;
>  	ssize_t name_sz;
> @@ -301,7 +351,7 @@ int mtd_virt_concat_create_join(void)
>  		 * concatenation are not handy, concat MTD device can't be
>  		 * created hence switch to next concat device.
>  		 */
> -		if (item->count != concat->num_subdev || item->vdev) {
> +		if (item->count != concat->num_subdev || item->vdev) {
>  			continue;
>  		} else {
>  			/* Calculate the legth of the name of the virtual device */
> @@ -309,10 +359,8 @@ int mtd_virt_concat_create_join(void)
>  				name_sz += (strlen(concat->subdev[idx]->name) + 1);
>  			name_sz += strlen(CONCAT_POSTFIX);
>  			name = kmalloc(name_sz + 1, GFP_KERNEL);
> -			if (!name) {
> -				mtd_virt_concat_put_mtd_devices(concat);
> +			if (!name)
>  				return -ENOMEM;
> -			}
>  
>  			ret = 0;
>  			for (idx = 0; idx < concat->num_subdev; idx++) {
> @@ -321,34 +369,40 @@ int mtd_virt_concat_create_join(void)
>  			}
>  			sprintf((name + ret), CONCAT_POSTFIX);
>  
> -			if (concat->mtd.name) {
> -				ret = memcmp(concat->mtd.name, name, name_sz);
> -				if (ret == 0) {
> -					kfree(name);
> -					continue;
> -				}
> +			vdev = kzalloc_flex(*vdev, parts, item->count);
> +			if (!vdev) {
> +				kfree(name);
> +				return -ENOMEM;
>  			}
> +			vdev->num_parts = item->count;
> +
>  			mtd = mtd_concat_create(concat->subdev, concat->num_subdev, name);
>  			if (!mtd) {
> +				kfree(vdev);
>  				kfree(name);
>  				return -ENXIO;
>  			}
> -			concat->mtd = *mtd;
>  			/* Arbitrary set the first device as parent */
> -			concat->mtd.dev.parent = concat->subdev[0]->dev.parent;
> -			concat->mtd.dev = concat->subdev[0]->dev;
> +			mtd->dev = concat->subdev[0]->dev;
> +			mtd->priv = vdev;
> +			mtd->_get_device = mtd_virt_concat_get_device;
> +			mtd->_put_device = mtd_virt_concat_put_device;
> +			vdev->mtd = mtd;
> +			kref_init(&vdev->refs);
> +			for (idx = 0; idx < item->count; idx++) {
> +				vdev->parts[idx] = concat->subdev[idx];
> +				kref_get(&vdev->parts[idx]->refcnt);
> +			}
>  
>  			/* Add the mtd device */
> -			ret = add_mtd_device(&concat->mtd);
> -			if (ret)
> -				goto destroy_concat;
> +			ret = add_mtd_device(mtd);
> +			if (ret) {
> +				kref_put(&vdev->refs, mtd_virt_concat_release);
> +				return ret;
> +			}
> +			item->vdev = vdev;
>  		}
>  	}
>  
>  	return 0;
> -
> -destroy_concat:
> -	mtd_concat_destroy(mtd);
> -
> -	return ret;
>  }

[Severity: High]
As reported by Sashiko on v2 [2], if partition registration or
mtd_virt_concat_create_join() fails during mtd_device_parse_register(),
can item->concat->subdev[idx] be left pointing to a partition whose master
mtd_info is about to be freed?

During mtd_device_parse_register(), add_mtd_partitions() passes matching
partitions to mtd_virt_concat_add(child), which stores child in
item->concat->subdev[idx], increments concat->num_subdev, and skips adding
child to parent->partitions:

drivers/mtd/mtdpart.c:add_mtd_partitions() {
    ...
    if (IS_REACHABLE(CONFIG_MTD_VIRT_CONCAT)) {
        if (mtd_virt_concat_add(child))
            continue;
    }
    ...
err_del_partitions:
    del_mtd_partitions(master);
    return ret;
}

If a subsequent partition in add_mtd_partitions() fails, err_del_partitions
only calls del_mtd_partitions(master) without calling
mtd_virt_concat_destroy(master). Likewise, if add_mtd_partitions()
succeeds and mtd_virt_concat_create_join() fails in
mtd_device_parse_register() (for example on kmalloc(), kzalloc_flex(),
mtd_concat_create(), or add_mtd_device()), mtd_device_parse_register()
jumps to out: without calling mtd_virt_concat_destroy(mtd) or
del_mtd_partitions(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))
        del_mtd_device(mtd);

    return ret;
}

Unlike physmap_flash_probe() (which calls physmap_flash_remove() and
mtd_device_unregister() on probe failure), most MTD drivers (such as
spi_nor_probe(), spinand_probe(), or brcmnand_init_cs()) do not call
mtd_device_unregister() when mtd_device_parse_register() returns an error,
and immediately free the master mtd_info. That leaves
item->concat->subdev[idx] pointing to a partition whose part->parent
references the freed master mtd_info:

1. When any other MTD master is later unregistered,
mtd_virt_concat_destroy() calls mtd_get_master(part) on every non-NULL
concat->subdev[idx], dereferencing the freed master mtd_info via
part->parent.

2. If the failed driver is bound again, the new check if
(concat->subdev[idx]) return false in mtd_virt_concat_add() rejects the
new partition (causing add_mtd_partitions() to register it standalone) and
mtd_virt_concat_create_join() then runs mtd_concat_create() on the stale
concat->subdev[idx] whose master was freed.

3. If mtd_virt_concat_create_join() succeeds for an earlier item in
concat_node_list and fails on a subsequent item, the earlier item's
item->vdev remains registered even though mtd_device_parse_register()
fails.

[1] https://lore.kernel.org/r/sashiko-outbox-193135@kernel.org
[2] https://lore.kernel.org/r/20261103080327.90193-1-izaiahsun@gmail.com

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

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