Re: [PATCH 03/58] mtd: add opt-in lifetime ownership for backing objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mtd: refcount imbalance in __put_mtd_device() when
  master->dev.type changes
- [Medium] mtd: mtd_add_partition() allows adding partitions to an
  unregistered master
- [Medium] mtd: add_mtd_device() leaves mtd->dev.type set when
  dev_set_name() fails
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] mtd: core: use-after-free in add_mtd_device() [1]
--

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

mtd: add opt-in lifetime ownership for backing objects

This commit introduces opt-in reference-counted lifetime management for
master MTD backing objects via mtd_set_release() and mtd_put_master(),
retaining the master and partition hierarchy while MTD handles, device
nodes, or OTP NVMEM providers remain active.

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 558a605ba7495..42c06a02cd99d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
[ ... ]
> @@ -748,16 +840,28 @@ int add_mtd_device(struct mtd_info *mtd)
[ ... ]
>  	mtd->index = i;
>  	kref_init(&mtd->refcnt);
> +	if (master->master.release) {
> +		mtd_get_lifetime(master);
> +		mtd->ref_master = master;
> +		if (mtd->parent && mtd->parent != master)
> +			get_device(&mtd->parent->dev);
> +	}

[Severity: Medium]
Can a partition still be added through mtd_add_partition() after the
parent or master has been unregistered?

If a process keeps an open file descriptor to /dev/mtd0 across driver
unbind (mtd_device_unregister() and mtd_put_master()) and issues
BLKPG_ADD_PARTITION, mtdchar_blkpg_ioctl() -> mtd_add_partition() calls
add_mtd_device(child) without checking whether parent->registered is still
true.  Because del_mtd_partitions() has already run, the new partition is
never cleaned up, leaking the partition, its MTD ID, THIS_MODULE, and
master.life_refcnt.  Furthermore, once the open handle on the managed
master is closed, mtd_device_release() leaves master->dev.type set, so
opening and closing the leaked child partition will do kref_get() on a
zero master->refcnt and call mtd_device_release() a second time.

> @@ -839,29 +946,38 @@ int add_mtd_device(struct mtd_info *mtd)
>  		}
>  	}
>  
> -	/* We _know_ we aren't being removed, because
> -	   our caller is still holding us here. So none
> -	   of this try_ nonsense, and no bitching about it
> -	   either. :) */
> -	__module_get(THIS_MODULE);
>  	return 0;
>  
>  fail_nvmem_add:
>  	device_del(&mtd->dev);
>  fail_added:
>  	/*
> -	 * Clear type and set nop release to prevent mtd_release() ->
> +	 * Clear type and override release to prevent mtd_release() ->
>  	 * release_mtd_partition() -> free_partition() from freeing mtd.
>  	 * The caller handles cleanup on failure.
>  	 */
>  	mtd->dev.type = NULL;
> -	mtd->dev.release = mtd_dev_release_nop;
> +	mtd->dev.release = mtd_dev_release_done;
> +	dev_set_drvdata(&mtd->dev, &released);
> +	wait_for_release = true;
>  	put_device(&mtd->dev);
>  	of_node_put(mtd_get_of_node(mtd));
>  fail_devname:
>  	idr_remove(&mtd_idr, i);
> +	ida_free(&mtd_ids, i);
> +	if (mtd->ref_master) {
> +		mtd->ref_master = NULL;
> +		if (mtd->parent && mtd->parent != master)
> +			put_device(&mtd->parent->dev);
> +		mtd_put_lifetime(master);
> +	}

[Severity: Critical]
This problem wasn't introduced by this patch, but can add_mtd_device()
trigger a use-after-free on the device_node [1] when
mtd_check_of_node() matches a partition node?

drivers/mtd/mtdcore.c:mtd_check_of_node() {
    ...
		if (plen == mtd_name_len &&
		    !strncmp(mtd->name, pname + offset, plen)) {
			mtd_set_of_node(mtd, mtd_dn);
			of_node_put(mtd_dn);
			break;
		}
    ...
}

mtd_check_of_node() stores mtd_dn in mtd and immediately drops its
reference via of_node_put(mtd_dn) before add_mtd_device() calls
of_node_get(mtd_get_of_node(mtd)).  In a dynamic DT environment, if the
node is removed in that window, of_node_put() frees mtd_dn and
of_node_get() dereferences freed memory.

[Severity: Medium]
Does jumping to fail_devname when dev_set_name() fails leave mtd->dev.type
set to &mtd_devtype?

mtd->dev.type is assigned right before dev_set_name(), and clearing it only
happens under fail_added.  Leaving mtd->dev.type non-NULL causes any retry
of add_mtd_device() to fail at WARN_ONCE(mtd->dev.type, ...), and causes
__put_mtd_device() on an open partition to call kref_put(&master->refcnt,
mtd_device_release) on a master whose struct device was never registered.

[ ... ]

> @@ -1348,13 +1484,14 @@ int __get_mtd_device(struct mtd_info *mtd)
>  		return -ENODEV;
>  	}
>  
> +	mtd_get_lifetime(master);
>  	while (mtd) {
>  		if (mtd != master)
>  			kref_get(&mtd->refcnt);
>  		mtd = mtd->parent;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> +	if (master->dev.type)
>  		kref_get(&master->refcnt);
>  
>  	return 0;
> @@ -1441,6 +1578,7 @@ void __put_mtd_device(struct mtd_info *mtd)
>  	struct mtd_info *master = mtd_get_master(mtd);
>  	/* master can be freed in _put_device */
>  	struct module *owner = master->owner;
> +	bool managed = master->master.release;
>  
>  	while (mtd) {
>  		/* kref_put() can relese mtd, so keep a reference mtd->parent */
> @@ -1451,12 +1589,14 @@ void __put_mtd_device(struct mtd_info *mtd)
>  		mtd = parent;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> +	if (master->dev.type)
>  		kref_put(&master->refcnt, mtd_device_release);

[Severity: Medium]
Can master->dev.type change from NULL to non-NULL between
__get_mtd_device() and __put_mtd_device(), causing an unmatched
kref_put(&master->refcnt, mtd_device_release)?

When CONFIG_MTD_PARTITIONED_MASTER=n, master->dev.type is NULL while
mtd_device_parse_register() runs parse_mtd_partitions().  If a first
partition is registered and opened by a notifier or userspace (seeing
master->dev.type == NULL in __get_mtd_device()), and a later partition
fails to register in add_mtd_partitions(), del_mtd_partitions() unregisters
the first partition while it remains open:

drivers/mtd/mtdcore.c:mtd_device_parse_register() {
    ...
	ret = parse_mtd_partitions(mtd, types, parser_data);
	if (ret == -EPROBE_DEFER)
		goto out;

	if (ret > 0)
		ret = 0;
	else if (nr_parts)
		ret = add_mtd_partitions(mtd, parts, nr_parts);
	else if (!device_is_registered(&mtd->dev))
		ret = add_mtd_device(mtd);
    ...
}

When nr_parts == 0 and parse_mtd_partitions() returns a non-defer error,
mtd_device_parse_register() falls back to add_mtd_device(mtd), which sets
master->dev.type = &mtd_devtype and initializes master->refcnt to 1.
Closing the open partition handle then sees master->dev.type != NULL in
__put_mtd_device() and drops master->refcnt to 0 while the master is still
registered.

[1] https://sashiko.dev/#/bug/linux-11fdb383-132f-473b-9284-101b4753fcd6

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-submit-rawnand-retained-handle-lifetime-v1-v1-0-0181ef0cc4c8@gmail.com?part=3

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic