Re: [PATCH v4 03/58] mtd: separate class devices from retained backing objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mtd: out-of-bounds access in gluebi_get_device() via
  add_mtd_partitions()
- [Medium] mtd: concurrent child removal in add_mtd_partitions() deletes
  all partitions
- [Medium] mtd: mtd_device_parse_register() and unregister check
  device_is_registered()
--

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

mtd: separate class devices from retained backing objects

This commit allocates MTD class devices separately from struct mtd_info,
moves the MTD class and device release handling into a built-in helper,
and adds reference counting for backing master and partition metadata.

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 620326279957f..dd404d36156ea 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
[ ... ]
> @@ -1150,7 +1200,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		ret = 0;
>  	else if (nr_parts)
>  		ret = add_mtd_partitions(mtd, parts, nr_parts);
> -	else if (!device_is_registered(&mtd->dev))
> +	else if (!mtd->dev || !device_is_registered(mtd->dev))
>  		ret = add_mtd_device(mtd);
>  	else
>  		ret = 0;
> @@ -1184,7 +1234,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		nvmem_unregister(mtd->otp_factory_nvmem);
>  	}
>  
> -	if (ret && device_is_registered(&mtd->dev)) {
> +	if (ret && mtd->dev && device_is_registered(mtd->dev)) {
>  		err = del_mtd_device(mtd);
>  		if (err)
>  			pr_err("Error when deleting MTD device (%d)\n", err);
> @@ -1225,7 +1275,7 @@ int mtd_device_unregister(struct mtd_info *master)
>  	if (err)
>  		return err;
>  
> -	if (!device_is_registered(&master->dev))
> +	if (!master->dev || !device_is_registered(master->dev))
>  		return 0;
>  
>  	return del_mtd_device(master);

[Severity: Medium]
Can checking device_is_registered(master->dev) instead of
master->registered cause mtd_device_unregister() to call del_mtd_device()
a second time and fail with -ENODEV?

With CONFIG_MTD_PARTITIONED_MASTER=y, mtd_device_parse_register() calls
add_mtd_device(mtd) before parsing and registering partitions.  If
userspace (such as udevd on KOBJ_ADD) or an in-kernel consumer acquires a
handle on the master during that window and partition registration then
fails (for example with -EPROBE_DEFER), del_mtd_device(mtd) clears
mtd->registered and drops the initial refcnt without running
mtd_device_release().  Because mtd->dev remains registered until the
handle is put, a driver error path that calls mtd_device_unregister()
(such as physmap_flash_probe() -> physmap_flash_remove()) sees
device_is_registered(master->dev) == true and calls del_mtd_device(master)
again, returning -ENODEV.  Wait-free unmanaged callers also return from
mtd_device_parse_register() with mtd->dev drvdata and mtd->nvmem still
attached to the mtd_info they are about to free, and a deferred probe
retry hits WARN_ONCE(mtd->dev) in add_mtd_device().

[ ... ]

> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index c316cfe92c7e4..58d1343476912 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
[ ... ]
> @@ -419,35 +420,47 @@ int add_mtd_partitions(struct mtd_info *parent,
>  	       nbparts, parent->name);
>  
>  	for (i = 0; i < nbparts; i++) {
> +		mutex_lock(&master->master.partitions_lock);
> +		if (master->master.unregistering ||
> +		    (mtd_is_partition(parent) && !parent->registered)) {
> +			ret = -ENODEV;
> +			goto err_unlock;
> +		}
> +
>  		child = allocate_partition(parent, parts + i, i, cur_offset);
>  		if (IS_ERR(child)) {
>  			ret = PTR_ERR(child);
> -			goto err_del_partitions;
> +			goto err_unlock;
>  		}
>  
>  		if (IS_REACHABLE(CONFIG_MTD_VIRT_CONCAT)) {
> -			if (mtd_virt_concat_add(child))
> +			if (mtd_virt_concat_add(child)) {
> +				mutex_unlock(&master->master.partitions_lock);
>  				continue;
> +			}
>  		}
>  
> -		mutex_lock(&master->master.partitions_lock);
>  		list_add_tail(&child->part.node, &parent->partitions);
> -		mutex_unlock(&master->master.partitions_lock);
>  
>  		ret = add_mtd_device(child);
>  		if (ret) {
> -			mutex_lock(&master->master.partitions_lock);
>  			list_del(&child->part.node);
> -			mutex_unlock(&master->master.partitions_lock);
> -
>  			free_partition(child);
> -			goto err_del_partitions;
> +			goto err_unlock;
>  		}
>  
>  		mtd_add_partition_attrs(child);
> +		cur_offset = child->part.offset + child->part.size;
> +
> +		/* Parsing drops the partition lock and may race with deletion. */
> +		ret = PTR_ERR_OR_ZERO(get_mtd_device(child, -1));
> +		if (ret)
> +			goto err_unlock;
> +		mutex_unlock(&master->master.partitions_lock);
>  
>  		/* Look for subpartitions (skip if no maching parser found) */
>  		ret = parse_mtd_partitions(child, parts[i].types, NULL);
> +		put_mtd_device(child);

[Severity: High]
Does calling get_mtd_device(child, -1) on every new partition cause an
out-of-bounds access in gluebi_get_device() when a gluebi MTD device is
partitioned?

__get_mtd_device(child) passes child to master->_get_device(mtd), whereas
__put_mtd_device(child) passes master to master->_put_device(master).
gluebi_get_device() applies container_of() directly to mtd without calling
mtd_get_master(mtd):

drivers/mtd/ubi/gluebi.c:gluebi_get_device() {
    ...
	gluebi = container_of(mtd, struct gluebi_device, mtd);
	mutex_lock(&devices_mutex);
	if (gluebi->refcnt > 0) {
		gluebi->refcnt += 1;
    ...
}

Because child is allocated by allocate_partition() as a standalone struct
mtd_info rather than embedded in struct gluebi_device, gluebi_get_device()
reads and writes past the end of child, and the paired put_mtd_device(child)
then decrements the master gluebi->refcnt in gluebi_put_device(master).

>  		if (ret < 0 && ret == -ENOENT) {
>  			pr_debug("Skip parsing subpartitions: %d\n", ret);
>  			continue;
> @@ -455,12 +468,12 @@ int add_mtd_partitions(struct mtd_info *parent,
>  			pr_err("Failed to parse subpartitions: %d\n", ret);
>  			goto err_del_partitions;
>  		}
> -
> -		cur_offset = child->part.offset + child->part.size;
>  	}
>  
>  	return 0;
>  
> +err_unlock:
> +	mutex_unlock(&master->master.partitions_lock);
>  err_del_partitions:
>  	del_mtd_partitions(master);

[Severity: Medium]
If child is removed via BLKPG_DEL_PARTITION while partitions_lock is
dropped for parse_mtd_partitions(child, ...), does jumping to err_unlock
destroy all partitions on the root master device?

When a subpartition parser calls add_mtd_partitions(child, ...) after
child->registered has been cleared by mtd_del_partition(), the
!parent->registered check sets ret = -ENODEV and jumps to err_unlock.
That falls through to del_mtd_partitions(master) on the top-level master
instead of just skipping subpartitions for the removed child, and returns
-ENODEV to the outer add_mtd_partitions() loop (which only ignores
-ENOENT), failing the entire master registration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-submit-rawnand-retained-handle-lifetime-v1-v4-0-556b1d7c6221@gmail.com?part=3

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