Re: [PATCH v2 1/2] mtd: core: add release hook and removed flag for unbind with open handles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] mtd: Device Model lifecycle violation causing sysfs Use-After-Free
- [High] mtd: Use-After-Free of master object in __put_mtd_device()
- [High] mtd: Master driver object memory leak when CONFIG_MTD_PARTITIONED_MASTER is disabled
- [High] mtd: lockless TOCTOU race on master->removed allows UAF on unbind
--

commit 5c236b852da3e75f7d2ec6f13275ac5b432bb26f
Author: Tze Yee Ng <tze.yee.ng@altera.com>

    mtd: core: add release hook and removed flag for unbind with open handles

This commit introduces a new _free hook to struct mtd_info called from the
kref release function to ensure the backing driver object is destroyed only
when the last reference drops. It also adds a removed flag to struct mtd_info
which is set on unbind and checked before performing hardware operations to
prevent Use-After-Free of parent devm resources.

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 491a27d0315fa..59133894b5e5c 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -118,6 +118,7 @@ static void mtd_device_release(struct kref *kref)
>  {
>  	struct mtd_info *mtd = container_of(kref, struct mtd_info, refcnt);
>  	bool is_partition = mtd_is_partition(mtd);
> +	void (*mtd_free)(struct mtd_info *mtd) = mtd->_free;
>  
>  	debugfs_remove_recursive(mtd->dbg.dfs_dir);
>  
> @@ -135,6 +136,9 @@ static void mtd_device_release(struct kref *kref)
>  		memset(&mtd->dev, 0, sizeof(mtd->dev));
>  
>  	module_put(THIS_MODULE);
> +
> +	if (mtd_free)
> +		mtd_free(mtd);

[Severity: Critical]
Since struct mtd_info embeds a struct device, does freeing it here violate the
Device Model lifecycle?

If user-space holds an open sysfs file for the device, the device core retains
an elevated kobject reference count on mtd->dev even after device_unregister()
returns.

If mtd_free() destroys the memory containing the embedded struct device, won't
the device core access freed memory when put_device() is eventually called upon
sysfs file closure?


[Severity: High]
Does this new mtd_free() hook cause a Use-After-Free in __put_mtd_device()?

Looking at __put_mtd_device() in drivers/mtd/mtdcore.c, we have:

	if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
		kref_put(&master->refcnt, mtd_device_release);

	module_put(master->owner);

	/* must be the last as master can be freed in the _put_device */
	if (master->_put_device)
		master->_put_device(master);

If mtd_device_release() synchronously frees the master object, won't the
subsequent dereferences of master->owner and master->_put_device access
freed memory?


[Severity: High]
Will the master driver object leak if CONFIG_MTD_PARTITIONED_MASTER is disabled?

When CONFIG_MTD_PARTITIONED_MASTER is disabled, the master device itself is
never registered via add_mtd_device(), so its refcount isn't incremented by
openers or managed by __get_mtd_device() or __put_mtd_device().

If drivers no longer free their objects in the remove() callback and rely
entirely on mtd_device_release(), how will the master driver object be freed
when CONFIG_MTD_PARTITIONED_MASTER is disabled?

>  }
>  
>  #define MTD_DEVICE_ATTR_RO(name) \

[ ... ]

> @@ -1471,6 +1478,9 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
>  	adjinstr = *instr;
>  
> +	if (master->removed)
> +		return -ENODEV;
> +

[Severity: High]
Is this lockless check sufficient to prevent the Use-After-Free this patch
aims to solve?

If mtd_erase() checks master->removed and it evaluates to false, the thread
could be preempted.

If a concurrent driver unbind executes the driver's remove() callback,
drains in-flight operations, sets master->removed = true, and then the device
model tears down devm-managed resources, wouldn't the preempted MTD thread
resume and call master->_erase() on freed devm memory and a powered-down bus?

>  	if (!mtd->erasesize || !master->_erase)
>  		return -ENOTSUPP;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791512623.git.tze.yee.ng@altera.com?part=1