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

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

[Yuqiang Sun]

Partitions that are part of a virtual concatenation are never registered
on their own: add_mtd_partitions() hands them to mtd_virt_concat_add() and
skips them, so they are not on the parent's partition list either. The
concatenated device is registered with a copy of the first partition's
struct device, whose parent is the master's device.

When the master is unregistered, mtd_virt_concat_destroy() looks for a
concat item whose DT nodes include the master's of_node. The items hold
the partition nodes, so nothing ever matches: the concat stays registered
and keeps the master's device alive (it stays in the idr and in
/proc/mtd), while the driver frees the master. Reading /proc/mtd then
uses freed memory, and so does device_shutdown(). The concat's partitions
are leaked, and since the item's subdev count is never reset, binding the
driver again writes past the end of concat->subdev[].

The matching branch is reached only from the MTD core's module exit,
which cannot run while the concat is registered (it holds a reference on
the module). There, a failed add_mtd_device() frees the caller's mtd
instead of the child. The concat also puts references on its parts that
it never took.

Find the concats to tear down by the master of their parts instead.
Unregister each one that includes a partition of the master that goes
away, free those partitions and drop them from the item, which keeps its
DT nodes and the other parts, so the concat is joined again when the
master comes back. Do not put references that were never taken, and only
unregister a concat at module exit if it is registered.

For this to work when the parts are on different masters, and when the
concat is in use as its master goes away:
- Keep each part in the slot of its DT node, rather than in the order the
  parts are registered, so that a part whose master comes back does not
  end up last.
- Register the device that mtd_concat_create() returns, instead of a
  copy of it in the item: an item can then be joined again while the old
  device is still open, and the error path of add_mtd_device() no longer
  frees the erase regions the copy still points to.
- Keep the device and its name until its last user puts it, through the
  _get_device() and _put_device() hooks.
- Count the references to each part, held by the item and by each
  device that includes it, so that a part is freed only once no device
  in use includes it: the other master can go away too, or the item can
  be joined again, while the old device is open. The parts left in the
  items are freed at module exit.

Tested on QEMU (arm64 virt, TCG) with KASAN, physmap and the MTD core as
modules, with two partitions of the CFI flash joined by
part-concat-next, and with one partition on each of the two flash banks
described as two flash devices: unbind the first flash, bind it again,
unbind it again, unload the modules. Without this patch the first unbind
leaves the devices behind and each read of /proc/mtd gives KASAN
slab-use-after-free reports in mtd_proc_show(), binding again gives
slab-out-of-bounds writes in mtd_virt_concat_add(), the MTD core cannot
be unloaded, and device_shutdown() at power-off reports use-after-free
and refcount_t underflow. With it the concat is removed on unbind,
created again with its parts in order and readable after binding, and
the MTD core unloads, with no reports. Failing each of the first eight
allocations under mtd_virt_concat_create_join() when binding again fails
the bind, with no reports. The same steps without part-concat-next are
clean with and without this patch.

Fixes: 43db6366fc2d ("mtd: Add driver for concatenating devices")
Cc: stable@vger.kernel.org
Signed-off-by: Yuqiang Sun <izaiahsun@gmail.com>
---

Notes:
    The bug was found with kbound, a research static analysis prototype
    (its report was the add_mtd_device() failure path in
    mtd_virt_concat_destroy(); running it showed the teardown problem).
    The patch, the changelog and the test setup were written with the help
    of an AI coding assistant (Claude); I have reviewed and tested them as
    described above.
    
    Changes in v3, after the Sashiko review of v2:
    - Count the references to each part instead of keeping only the parts
      of the master that went away with the open device. In v2 a part of
      the other master was freed when that master went away too, or when
      the item was joined again, while the old device still pointed to it.
    - Free the parts left in the items at module exit.
    
    Changes in v2, after the Sashiko review of v1:
    - Keep each part in the slot of its DT node. With a concat across two
      flash devices, v1 joined it again as kbb-kba-concat after the first
      flash was bound again.
    - Register the device returned by mtd_concat_create() instead of a copy
      in the item. This also removes the double free of the erase regions
      after a failed add_mtd_device(). That path is not reachable on QEMU,
      whose CFI flash has a single erase region, so it is not tested.
    - Keep the concat and its name until the last user puts the concat.
    
    An MTD device held open while its flash driver is unbound gives KASAN
    use-after-free reports in the MTD core and the CFI driver, with or
    without this patch, for a plain partition too: the driver frees the
    master while the open device, whose struct device has the master's as
    its parent, is still around. With this patch a concat held open across
    the unbind gets reports of the same kind, none of them in
    mtd_virt_concat.c. That includes holding the concat across the two
    flash devices open while both are unbound, or while the first is
    unbound, bound again and the second unbound, and then reading its
    second part: with v2 these also used the second part after
    mtd_virt_concat_destroy() had freed it. Closing the device then frees
    its parts.
    
    v2: https://lore.kernel.org/all/20261103080327.90193-1-izaiahsun@gmail.com/
    v1: https://lore.kernel.org/all/20261102083045.8512-1-izaiahsun@gmail.com/

 drivers/mtd/mtd_virt_concat.c | 228 +++++++++++++++++++++-------------
 include/linux/mtd/concat.h    |  14 +--
 2 files changed, 148 insertions(+), 94 deletions(-)

diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
index 40b300021dda..84d044bd00e9 100644
--- a/drivers/mtd/mtd_virt_concat.c
+++ b/drivers/mtd/mtd_virt_concat.c
@@ -26,15 +26,32 @@ static LIST_HEAD(concat_node_list);
  * @head: List handle
  * @count: Number of nodes
  * @nodes: Pointer to the nodes (partitions) to concatenate
- * @concat: Concatenation container
+ * @concat: Parts found so far: subdev[i] is the partition of nodes[i], or NULL.
+ *          Each holds a reference to its part.
+ * @vdev: The concatenated device, if it is registered
  */
 struct mtd_virt_concat_node {
 	struct list_head head;
 	unsigned int count;
 	struct mtd_concat *concat;
+	struct mtd_virt_concat_dev *vdev;
 	struct device_node *nodes[] __counted_by(count);
 };
 
+/**
+ * struct mtd_virt_concat_dev - A concatenated device
+ * @mtd: The device, from mtd_concat_create()
+ * @refs: One for the registration and one for each user of the device
+ * @num_parts: Number of entries in @parts
+ * @parts: The parts of the device, each holding a reference to its part
+ */
+struct mtd_virt_concat_dev {
+	struct mtd_info *mtd;
+	struct kref refs;
+	unsigned int num_parts;
+	struct mtd_info *parts[] __counted_by(num_parts);
+};
+
 /**
  * mtd_is_part_concat - Check if the device is already part
  *                       of a concatenated device
@@ -57,94 +74,122 @@ static bool mtd_is_part_concat(struct device_node *dev)
 	return false;
 }
 
-static void mtd_virt_concat_put_mtd_devices(struct mtd_concat *concat)
+/*
+ * A part is never registered on its own, so its refcnt, which the MTD core
+ * only uses for registered devices, counts the concat item and the
+ * concatenated devices that hold it. A part can outlive its master's
+ * unregistration in a concatenated device that is still in use.
+ */
+static void mtd_virt_concat_free_part(struct kref *refcnt)
 {
-	int i;
+	struct mtd_info *part = container_of(refcnt, struct mtd_info, refcnt);
+
+	kfree(part->name);
+	kfree(part);
+}
+
+static void mtd_virt_concat_put_part(struct mtd_info *part)
+{
+	kref_put(&part->refcnt, mtd_virt_concat_free_part);
+}
+
+static void mtd_virt_concat_release(struct kref *refs)
+{
+	struct mtd_virt_concat_dev *vdev =
+		container_of(refs, struct mtd_virt_concat_dev, refs);
+	unsigned int i;
+
+	kfree(vdev->mtd->name);
+	mtd_concat_destroy(vdev->mtd);
+	for (i = 0; i < vdev->num_parts; i++)
+		mtd_virt_concat_put_part(vdev->parts[i]);
+	kfree(vdev);
+}
+
+/*
+ * A concatenated device has no parent, so it is its own master: the MTD core
+ * calls these for each of its users, the last _put_device() after it is done
+ * with the device.
+ */
+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);
+	}
 }
 
 void mtd_virt_concat_destroy_joins(void)
 {
-	struct mtd_virt_concat_node *item, *tmp;
-	struct mtd_info *mtd;
+	struct mtd_virt_concat_node *item;
+	int idx;
 
-	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);
+		for (idx = 0; idx < item->count; idx++) {
+			if (item->concat->subdev[idx])
+				mtd_virt_concat_put_part(item->concat->subdev[idx]);
 		}
+		kfree(item->concat);
+		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 once no
+ * concatenated device that is still in use holds 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_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;
 }
 
 /**
@@ -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) {
+		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;
 }
diff --git a/include/linux/mtd/concat.h b/include/linux/mtd/concat.h
index f8d4d6ac1fc1..4c09b4f5b6e9 100644
--- a/include/linux/mtd/concat.h
+++ b/include/linux/mtd/concat.h
@@ -62,16 +62,16 @@ bool mtd_virt_concat_add(struct mtd_info *mtd);
 int mtd_virt_concat_create_join(void);
 
 /**
- * mtd_virt_concat_destroy - Remove the concat that includes a specific mtd device
- *                           as one of its components.
+ * mtd_virt_concat_destroy - Remove the concats that include partitions of a
+ *                           specific mtd device as components.
  * @mtd: pointer to MTD device info structure.
  *
- * Returns 0 on succes, or a negative error code.
- *
- * If the mtd_info object is part of a concatenated device, all other MTD devices
- * within that concat are registered individually. The concatenated device is then
- * removed, along with its concatenation component.
+ * Returns 0.
  *
+ * Called when the master of @mtd is unregistered: every concatenated device
+ * with a partition of that master is unregistered, and those partitions are
+ * freed once it is no longer in use. The concatenation is created again when
+ * all its parts are back.
  */
 int mtd_virt_concat_destroy(struct mtd_info *mtd);
 

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.53.0


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

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

[sashiko-bot]

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/