Re: [PATCH v2 04/58] mtd: retain virtual concatenations across component removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] mtd: UAF in mtd_virt_concat_part_release() on unmanaged
  physical master
- [High] mtd: parenting concat device to parts[0]->dev.parent causes
  parent UAF
- [Medium] mtd: uninitialized part.node triggers WARN_ON in
  release_mtd_partition()
- [Medium] mtd: mtd_virt_concat_register() drops of_node from
  concatenated device
--

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

mtd: retain virtual concatenations across component removal

This commit refactors virtual MTD concatenation staging and lifetime
management so hidden partition components and published concatenations are
retained across component removal and cleaned up on registration failure.

Link: https://lore.kernel.org/r/20261103143953.69642-1-izaiahsun@gmail.com/
Link: https://lore.kernel.org/r/sashiko-outbox-195551@kernel.org/

> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> index da4277ced4d63..1c797a4afb9ba 100644
> --- a/drivers/mtd/mtd_virt_concat.c
> +++ b/drivers/mtd/mtd_virt_concat.c

[ ... ]

> +/* Hidden partitions have no registered device to own their storage. */
> +static void mtd_virt_concat_part_release(struct kref *ref)
> +{
> +	struct mtd_info *mtd = container_of(ref, struct mtd_info, refcnt);
> +	struct mtd_info *master = mtd_get_master(mtd);
> +	struct mtd_info *parent = mtd->parent;
> +
> +	of_node_put(mtd_get_of_node(mtd));
> +	release_mtd_partition(mtd);

[Severity: Medium]
Does calling release_mtd_partition() on a hidden concatenation component
always trigger WARN_ON(!list_empty(&mtd->part.node))?

allocate_partition() leaves child->part.node zero-initialized, and
add_mtd_partitions() skips list_add_tail(&child->part.node,
&parent->partitions) when mtd_virt_concat_add() claims the partition.
Because mtd->part.node is never initialized with INIT_LIST_HEAD(),
list_empty(&mtd->part.node) evaluates to false whenever a staged or
published component is released.

> +	if (parent != master)
> +		put_device(&parent->dev);
> +	mtd_put_lifetime(master);
> +}

[Severity: High]
Can mtd_virt_concat_part_release() cause a use-after-free when the backing
physical master does not use mtd_set_release()?

Master lifetime management is opt-in, so add_mtd_device() and mtd_release()
only retain and release master and parent when master->master.release is set
(recording it in mtd->ref_master).  Here, if an unmanaged physical master is
unregistered and freed while a consumer holds an open handle to the virtual
concatenation, closing that handle later walks mtd_get_master(mtd) and reads
master->master.release in mtd_put_lifetime(master) on the freed master.

[ ... ]

> +	mtd = mtd_concat_create(parts, item->count, name);
> +	if (!mtd) {
> +		ret = -ENXIO;
> +		goto out_device;
> +	}
> +	for (i = 0; i < item->count; i++)
> +		kref_get(&parts[i]->refcnt);
> +	mtd->dev.parent = parts[0]->dev.parent;

[Severity: High]
Does setting mtd->dev.parent to parts[0]->dev.parent leave the concatenated
device attached to a parent struct device whose registration lifetime is not
pinned?

allocate_partition() sets parts[0]->dev.parent to the lead physical master's
&master->dev (or an outer partition's &parent->dev), but the virtual
concatenation is its own master (mtd->parent is NULL) and never increments
that parent MTD's refcnt.  If the lead physical master is unregistered while
the virtual concatenation is held open, mtd_device_release() calls
device_unregister() on the physical master's struct device (and zeroes it
with memset() when unmanaged) while the concatenated device and its mtdXro
companion are still registered children of it.

> +	mtd->owner = THIS_MODULE;
> +	mtd->priv = device;
> +	mtd_set_dev_defaults(mtd);
> +	lockdep_set_class(&mtd->master.partitions_lock, &concat_partitions_lock_key);
> +	mtd_set_release(mtd, mtd_virt_concat_release);
> +	device->mtd = mtd;
> +	ret = add_mtd_device(mtd);

[Severity: Medium]
Does registering the mtd_concat_create() device directly without copying
parts[0]'s of_node leave mtd->dev.of_node NULL on the concatenated device?

Previously, copying concat->subdev[0]->dev preserved dev.of_node on
concat->mtd.  Because the concatenated device is not a partition,
mtd_find_of_node() returns NULL in add_mtd_device() unless mtd_set_of_node()
was called beforehand, which breaks DT consumers of the concatenated device
such as mtdN alias assignment, linux,rootfs, and mtd_nvmem_add().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-submit-rawnand-retained-handle-lifetime-v1-v2-0-9137d08f05df@gmail.com?part=4

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