[PATCH 0/6] cdev: delete partitions when deleting master cdev

[PATCH 0/6] cdev: delete partitions when deleting master cdev

[Ahmad Fatoum]

blockdevice_unregister only calls devfs_remove on the root cdev and
leaves the partition cdevs dangling. This doesn't break until the
block device parent struct device is freed at which time, it will
iterate over its cdevs to free them. If there's partitions there,
list_del on the partitions triggers a use after free.

This series fixes this by removing partitions whenever the master cdev
is deleted.

Code has been this way since for ever, but virtio deletes its devices on
shutdown triggering this issue. As virtio isn't that critical, I think
it's ok to not go into master right away and sit in next first.

 common/partitions.c    | 12 +++++++----
 drivers/base/driver.c  |  2 +-
 drivers/of/partition.c | 16 +++++++--------
 fs/devfs-core.c        | 45 +++++++++++++++++++++++++++---------------
 include/driver.h       | 12 +++++++----
 lib/bootstrap/devfs.c  |  2 +-
 6 files changed, 55 insertions(+), 34 deletions(-)

-- 
2.39.2

[PATCH 1/6] cdev: make __devfs_add_partition's last argument optional

[Ahmad Fatoum]

Follow-up commit will expose __devfs_add_partition after rename to more
users. Most of these don't care about the last parameter, so accept
NULL to indicate that it won't be read back.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/devfs-core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index 4e16d55e367e..6abf5eacc8dd 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -478,6 +478,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
 		const struct devfs_partition *partinfo, loff_t *end)
 {
 	loff_t offset, size;
+	loff_t _end = end ? *end : 0;
 	static struct cdev *new;
 	struct cdev *overlap;
 
@@ -488,7 +489,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
 		offset = partinfo->offset;
 	else if (partinfo->offset == 0)
 		/* append to previous partition */
-		offset = *end;
+		offset = _end;
 	else
 		/* relative to end of cdev */
 		offset = cdev->size + partinfo->offset;
@@ -498,13 +499,15 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
 	else
 		size = cdev->size + partinfo->size - offset;
 
-	if (offset >= 0 && offset < *end)
+	if (offset >= 0 && offset < _end)
 		pr_debug("partition %s not after previous partition\n",
 				partinfo->name);
 
-	*end = offset + size;
+	_end = offset + size;
+	if (end)
+		*end = _end;
 
-	if (offset < 0 || *end > cdev->size) {
+	if (offset < 0 || _end > cdev->size) {
 		pr_warn("partition %s not completely inside device %s\n",
 				partinfo->name, cdev->name);
 		return ERR_PTR(-EINVAL);
@@ -558,7 +561,6 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 		loff_t size, unsigned int flags, const char *name)
 {
 	struct cdev *cdev;
-	loff_t end = 0;
 	const struct devfs_partition partinfo = {
 		.offset = offset,
 		.size = size,
@@ -570,7 +572,7 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 	if (!cdev)
 		return ERR_PTR(-ENOENT);
 
-	return __devfs_add_partition(cdev, &partinfo, &end);
+	return __devfs_add_partition(cdev, &partinfo, NULL);
 }
 
 int devfs_del_partition(const char *name)
-- 
2.39.2

[PATCH 2/6] driver.h: move devfs_add/del_partition later in file

[Ahmad Fatoum]

This groups the function with similar functions and prepares adding
variants that take a struct devfs_partition argument, as that's only
defined later.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/driver.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/driver.h b/include/driver.h
index a0234fb6c31b..0e3d5635e4c1 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -631,10 +631,6 @@ cdev_find_child_by_gpt_typeuuid(struct cdev *cdev, guid_t *typeuuid)
 	return ERR_PTR(-ENOENT);
 }
 
-struct cdev *devfs_add_partition(const char *devname, loff_t offset,
-		loff_t size, unsigned int flags, const char *name);
-int devfs_del_partition(const char *name);
-
 #ifdef CONFIG_FS_AUTOMOUNT
 void cdev_create_default_automount(struct cdev *cdev);
 #else
@@ -681,6 +677,10 @@ struct devfs_partition {
 int devfs_create_partitions(const char *devname,
 		const struct devfs_partition partinfo[]);
 
+struct cdev *devfs_add_partition(const char *devname, loff_t offset,
+		loff_t size, unsigned int flags, const char *name);
+int devfs_del_partition(const char *name);
+
 #define of_match_ptr(compat) \
 	IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL
 
-- 
2.39.2

[PATCH 3/6] cdev: export cdevfs_add_partition for general use

[Ahmad Fatoum]

__devfs_add_partition was added, so it can internally be used by
functions that already have a cdev and don't require looking it up first
by file path. This is useful elsewhere as well, so give it a less
internal-sounding name and start exporting it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/devfs-core.c  | 6 +++---
 include/driver.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index 6abf5eacc8dd..912bfeede8dc 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -474,7 +474,7 @@ static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t of
 	return ret ? ERR_PTR(ret) : cpart;
 }
 
-static struct cdev *__devfs_add_partition(struct cdev *cdev,
+struct cdev *cdevfs_add_partition(struct cdev *cdev,
 		const struct devfs_partition *partinfo, loff_t *end)
 {
 	loff_t offset, size;
@@ -572,7 +572,7 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 	if (!cdev)
 		return ERR_PTR(-ENOENT);
 
-	return __devfs_add_partition(cdev, &partinfo, NULL);
+	return cdevfs_add_partition(cdev, &partinfo, NULL);
 }
 
 int devfs_del_partition(const char *name)
@@ -618,7 +618,7 @@ int devfs_create_partitions(const char *devname,
 	for (; partinfo->name; ++partinfo) {
 		struct cdev *new;
 
-		new = __devfs_add_partition(cdev, partinfo, &offset);
+		new = cdevfs_add_partition(cdev, partinfo, &offset);
 		if (IS_ERR(new))
 			return PTR_ERR(new);
 
diff --git a/include/driver.h b/include/driver.h
index 0e3d5635e4c1..373c4f1d0bca 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -681,6 +681,9 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 		loff_t size, unsigned int flags, const char *name);
 int devfs_del_partition(const char *name);
 
+struct cdev *cdevfs_add_partition(struct cdev *cdev,
+		const struct devfs_partition *partinfo, loff_t *end);
+
 #define of_match_ptr(compat) \
 	IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL
 
-- 
2.39.2

[PATCH 4/6] partition: switch to using cdevfs_add_partition

[Ahmad Fatoum]

We already have a cdev, so it's wasteful to do a lookup by cdev name
using devfs_add_partition. Use the newly exported cdevfs_add_partition
helper directly instead.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/partitions.c    | 12 ++++++++----
 drivers/of/partition.c | 16 ++++++++--------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/common/partitions.c b/common/partitions.c
index e3e8a9f3044d..8a245a1eee3d 100644
--- a/common/partitions.c
+++ b/common/partitions.c
@@ -32,17 +32,21 @@ static int register_one_partition(struct block_device *blk,
 {
 	char *partition_name;
 	int ret;
-	uint64_t start = part->first_sec * SECTOR_SIZE;
-	uint64_t size = part->size * SECTOR_SIZE;
 	struct cdev *cdev;
+	struct devfs_partition partinfo = {
+		.offset = part->first_sec * SECTOR_SIZE,
+		.size = part->size * SECTOR_SIZE,
+	};
 
 	partition_name = basprintf("%s.%d", blk->cdev.name, no);
 	if (!partition_name)
 		return -ENOMEM;
+
+	partinfo.name = partition_name;
+
 	dev_dbg(blk->dev, "Registering partition %s on drive %s (partuuid=%s)\n",
 				partition_name, blk->cdev.name, part->partuuid);
-	cdev = devfs_add_partition(blk->cdev.name,
-				start, size, 0, partition_name);
+	cdev = cdevfs_add_partition(&blk->cdev, &partinfo, NULL);
 	if (IS_ERR(cdev)) {
 		ret = PTR_ERR(cdev);
 		goto out;
diff --git a/drivers/of/partition.c b/drivers/of/partition.c
index 9b73419a83af..b91fe616d990 100644
--- a/drivers/of/partition.c
+++ b/drivers/of/partition.c
@@ -26,13 +26,12 @@ enum of_binding_name {
 
 struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
 {
+	struct devfs_partition partinfo = {};
 	const char *partname;
 	char *filename;
 	struct cdev *new;
 	const __be32 *reg;
-	u64 offset, size;
 	int len;
-	unsigned long flags = 0;
 	int na, ns;
 
 	if (!node)
@@ -50,8 +49,8 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
 		return NULL;
 	}
 
-	offset = of_read_number(reg, na);
-	size = of_read_number(reg + na, ns);
+	partinfo.offset = of_read_number(reg, na);
+	partinfo.size = of_read_number(reg + na, ns);
 
 	partname = of_get_property(node, "label", NULL);
 	if (!partname)
@@ -59,14 +58,15 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
 	if (!partname)
 		return NULL;
 
-	debug("add partition: %s.%s 0x%08llx 0x%08llx\n", cdev->name, partname, offset, size);
+	debug("add partition: %s.%s 0x%08llx 0x%08llx\n", cdev->name, partname,
+	      partinfo.offset, partinfo.size);
 
 	if (of_get_property(node, "read-only", NULL))
-		flags = DEVFS_PARTITION_READONLY;
+		partinfo.flags = DEVFS_PARTITION_READONLY;
 
-	filename = basprintf("%s.%s", cdev->name, partname);
+	partinfo.name = filename = basprintf("%s.%s", cdev->name, partname);
 
-	new = devfs_add_partition(cdev->name, offset, size, flags, filename);
+	new = cdevfs_add_partition(cdev, &partinfo, NULL);
 	if (IS_ERR(new)) {
 		pr_err("Adding partition %s failed: %pe\n", filename, new);
 		new = NULL;
-- 
2.39.2

[PATCH 5/6] cdev: export and use cdevfs_del_partition

[Ahmad Fatoum]

Like what's the case with cdevfs_add_partition, a couple of users
already have a cdev, so it's wasteful to get its name and do a lookup
only to arrive at the same cdev. Export a cdevfs_del_partition that
directly works on the cdev and start using it instead.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/base/driver.c |  2 +-
 fs/devfs-core.c       | 23 +++++++++++++++--------
 include/driver.h      |  1 +
 lib/bootstrap/devfs.c |  2 +-
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 93607fc3b09d..4f18f5bb8123 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -295,7 +295,7 @@ int unregister_device(struct device *old_dev)
 	list_for_each_entry_safe(cdev, ct, &old_dev->cdevs, devices_list) {
 		if (cdev_is_partition(cdev)) {
 			dev_dbg(old_dev, "unregister part %s\n", cdev->name);
-			devfs_del_partition(cdev->name);
+			cdevfs_del_partition(cdev);
 		}
 	}
 
diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index 912bfeede8dc..fef66e08e293 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -575,17 +575,10 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 	return cdevfs_add_partition(cdev, &partinfo, NULL);
 }
 
-int devfs_del_partition(const char *name)
+int cdevfs_del_partition(struct cdev *cdev)
 {
-	struct cdev *cdev;
 	int ret;
 
-	cdev = cdev_by_name(name);
-	if (!cdev)
-		return -ENOENT;
-
-	if (!cdev_is_partition(cdev))
-		return -EINVAL;
 	if (cdev->flags & DEVFS_PARTITION_FIXED)
 		return -EPERM;
 
@@ -605,6 +598,20 @@ int devfs_del_partition(const char *name)
 	return 0;
 }
 
+int devfs_del_partition(const char *name)
+{
+	struct cdev *cdev;
+
+	cdev = cdev_by_name(name);
+	if (!cdev)
+		return -ENOENT;
+
+	if (!cdev_is_partition(cdev))
+		return -EINVAL;
+
+	return cdevfs_del_partition(cdev);
+}
+
 int devfs_create_partitions(const char *devname,
 		const struct devfs_partition partinfo[])
 {
diff --git a/include/driver.h b/include/driver.h
index 373c4f1d0bca..f4f26151e365 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -683,6 +683,7 @@ int devfs_del_partition(const char *name);
 
 struct cdev *cdevfs_add_partition(struct cdev *cdev,
 		const struct devfs_partition *partinfo, loff_t *end);
+int cdevfs_del_partition(struct cdev *cdev);
 
 #define of_match_ptr(compat) \
 	IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL
diff --git a/lib/bootstrap/devfs.c b/lib/bootstrap/devfs.c
index b127b27c1d58..b023da1f941e 100644
--- a/lib/bootstrap/devfs.c
+++ b/lib/bootstrap/devfs.c
@@ -150,7 +150,7 @@ void* bootstrap_read_devfs(char *devname, bool use_bb, int offset,
 	}
 
 delete_devfs_partition:
-	devfs_del_partition(partname);
+	cdevfs_del_partition(partition);
 
 	return result;
 }
-- 
2.39.2

[PATCH 6/6] cdev: delete partitions when deleting master cdev

[Ahmad Fatoum]

blockdevice_unregister only calls devfs_remove on the root cdev and
leaves the partition cdevs dangling. This doesn't break until the
block device parent struct device is freed at which time, it will
iterate over its cdevs to free them. If there's partitions there,
list_del on the partitions triggers a use after free.

Fix this by removing partitions whenever the master cdev is deleted.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/devfs-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index fef66e08e293..244f76f62c52 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -375,6 +375,7 @@ int devfs_create_link(struct cdev *cdev, const char *name)
 	}
 
 	INIT_LIST_HEAD(&new->links);
+	INIT_LIST_HEAD(&new->partitions);
 	list_add_tail(&new->list, &cdev_list);
 	list_add_tail(&new->link_entry, &cdev->links);
 
@@ -396,6 +397,9 @@ int devfs_remove(struct cdev *cdev)
 	list_for_each_entry_safe(c, tmp, &cdev->links, link_entry)
 		devfs_remove(c);
 
+	list_for_each_entry_safe(c, tmp, &cdev->partitions, partition_entry)
+		cdevfs_del_partition(c);
+
 	if (cdev_is_partition(cdev))
 		list_del(&cdev->partition_entry);
 
-- 
2.39.2

Re: [PATCH 3/6] cdev: export cdevfs_add_partition for general use

[Sascha Hauer]

On Wed, Jan 03, 2024 at 11:16:26AM +0100, Ahmad Fatoum wrote:
> __devfs_add_partition was added, so it can internally be used by
> functions that already have a cdev and don't require looking it up first
> by file path. This is useful elsewhere as well, so give it a less
> internal-sounding name and start exporting it.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  fs/devfs-core.c  | 6 +++---
>  include/driver.h | 3 +++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> index 6abf5eacc8dd..912bfeede8dc 100644
> --- a/fs/devfs-core.c
> +++ b/fs/devfs-core.c
> @@ -474,7 +474,7 @@ static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t of
>  	return ret ? ERR_PTR(ret) : cpart;
>  }
>  
> -static struct cdev *__devfs_add_partition(struct cdev *cdev,
> +struct cdev *cdevfs_add_partition(struct cdev *cdev,
>  		const struct devfs_partition *partinfo, loff_t *end)

Now that this function is exported the @end deserves a comment that
tells what it's good for. Alternatively, as the new users doesn't seem
to make use of this parameter, you could keep __devfs_add_partition
static call it from an exported wrapper function that passes NULL as
@end.

Sascha

>  {
>  	loff_t offset, size;
> @@ -572,7 +572,7 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
>  	if (!cdev)
>  		return ERR_PTR(-ENOENT);
>  
> -	return __devfs_add_partition(cdev, &partinfo, NULL);
> +	return cdevfs_add_partition(cdev, &partinfo, NULL);
>  }
>  
>  int devfs_del_partition(const char *name)
> @@ -618,7 +618,7 @@ int devfs_create_partitions(const char *devname,
>  	for (; partinfo->name; ++partinfo) {
>  		struct cdev *new;
>  
> -		new = __devfs_add_partition(cdev, partinfo, &offset);
> +		new = cdevfs_add_partition(cdev, partinfo, &offset);
>  		if (IS_ERR(new))
>  			return PTR_ERR(new);
>  
> diff --git a/include/driver.h b/include/driver.h
> index 0e3d5635e4c1..373c4f1d0bca 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -681,6 +681,9 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset,
>  		loff_t size, unsigned int flags, const char *name);
>  int devfs_del_partition(const char *name);
>  
> +struct cdev *cdevfs_add_partition(struct cdev *cdev,
> +		const struct devfs_partition *partinfo, loff_t *end);
> +
>  #define of_match_ptr(compat) \
>  	IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL
>  
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] fixup! cdev: export cdevfs_add_partition for general use

[Ahmad Fatoum]

Now that we export the function, some documentation can be useful.

Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/devfs-core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index 912bfeede8dc..060a24795c47 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -474,6 +474,17 @@ static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t of
 	return ret ? ERR_PTR(ret) : cpart;
 }
 
+/**
+ * cdevfs_add_partition() - add partition to already registered cdev
+ * @cdev: parent cdev
+ * @partinfo: new partition information
+ * @end: If not NULL, will hold the the (non-inclusive) end offset of the newly
+ *       created partition.
+ *
+ * Return: the newly registered cdev or an error pointer if the new partition
+ *         would clash with exisiting partitions or overflow the cdev.
+ *         A valid cdev can be freed with cdevfs_del_partition()
+ */
 struct cdev *cdevfs_add_partition(struct cdev *cdev,
 		const struct devfs_partition *partinfo, loff_t *end)
 {
-- 
2.39.2

Re: [PATCH 0/6] cdev: delete partitions when deleting master cdev

[Sascha Hauer]

On Wed, Jan 03, 2024 at 11:16:23AM +0100, Ahmad Fatoum wrote:
> blockdevice_unregister only calls devfs_remove on the root cdev and
> leaves the partition cdevs dangling. This doesn't break until the
> block device parent struct device is freed at which time, it will
> iterate over its cdevs to free them. If there's partitions there,
> list_del on the partitions triggers a use after free.
> 
> This series fixes this by removing partitions whenever the master cdev
> is deleted.
> 
> Code has been this way since for ever, but virtio deletes its devices on
> shutdown triggering this issue. As virtio isn't that critical, I think
> it's ok to not go into master right away and sit in next first.

Applied, thanks

Sascha

> 
>  common/partitions.c    | 12 +++++++----
>  drivers/base/driver.c  |  2 +-
>  drivers/of/partition.c | 16 +++++++--------
>  fs/devfs-core.c        | 45 +++++++++++++++++++++++++++---------------
>  include/driver.h       | 12 +++++++----
>  lib/bootstrap/devfs.c  |  2 +-
>  6 files changed, 55 insertions(+), 34 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |