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

[NG, TZE YEE]

On 9/10/2026 3:49 pm, sashiko-bot@xxxxxxxxxx wrote:

Hi maintainers,

Thanks for the review. For v3, I plan to drop the core release hook and 
the removed flag, and reduce patch 1 to a reorder in __put_mtd_device(). 
spi-nor would manage the object's lifetime with its own refcount.

> 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@xxxxxxxxxx>
>
>      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?

Not new: mtd_info is embedded in the driver object for every MTD driver 
and is already freed on unbind. v3 will frees it later than today, so it 
is not worse. I plan to leave the sysfs lifetime out of this series.

> [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?

Agreed. v3 will reads master->owner first, calls ->_put_device() last 
and drops the module reference after it.

> [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?

Agreed. This comes from relying on mtd_device_release() to free the
driver object. In v3 spi-nor would own a kref and drop the registration
reference unconditionally in remove(), independent of what
mtd_device_unregister() does.

> >   }
> >   
> >   #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?

Agreed. v3 would drop the flag from the core. spi-nor would set its own
removed flag under nor->lock in remove(), and prep_and_lock*() would
re-check it after taking the lock, so a waiter blocked on the lock sees
it and returns -ENODEV.

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

Thanks,
Tze Yee