[PATCH v3 2/2] mtd: spi-nor: fix use-after-free on unbind with open handles

[NG, TZE YEE]

Hi maintainers,

Thanks for the review. These four are real, but they are not new in this
series, and I'd prefer to keep it limited to the unbind use-after-free
on the I/O paths:


On 22/10/2026 12:18 pm, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] mtd: spi-nor: use-after-free in spi_nor_suspend() and spi_nor_resume()
> - [High] mtd: spi-nor: use-after-free of nor->dev in dev_dbg() after unregistration
> - [High] mtd: spi-nor: use-after-free of devres-backed nor->mtd.name after unbind
> - [High] mtd: spi-nor: missing nor->removed check in spi_nor_get_device() after unbind
> --
> 
> commit de3094832a517827e88d8d0203973cdc91471e95
> Author: Tze Yee Ng <tze.yee.ng at altera.com>
> 
> mtd: spi-nor: fix use-after-free on unbind with open handles
> 
> This commit moves spi_nor and its associated buffers off devres on the
> spi-mem probe path and refcounts struct spi_nor across registration and open
> MTD handles. During remove(), it unregisters debugfs and the MTD device, marks
> nor->removed under the lock to fence subsequent operations with -ENODEV, and
> drops the registration reference.
> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 8bc117b46e028..76928bd8413ed 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
> 
> [ ... ]
> 
>> @@ -3420,20 +3449,48 @@ static void spi_nor_resume(struct mtd_info *mtd)
>>   		dev_err(dev, "resume() failed\n");
>>   }
> 
> [Severity: Critical]
> What happens if the system suspends and resumes after spi_nor_remove() while
> an open file descriptor still holds a reference on the MTD device?
> 
> When a partition (or a master device with CONFIG_MTD_PARTITIONED_MASTER
> enabled) is open during spi_nor_remove(), del_mtd_device() drops mtd->refcnt
> from 2 to 1 without running mtd_device_release() -> device_unregister(), so
> mtd->dev remains registered on mtd_class with mtd_cls_pm_ops active:
> 
>    mtd_cls_suspend() -> mtd_suspend() -> spi_nor_suspend()
>    mtd_cls_resume()  -> mtd_resume()  -> spi_nor_resume()
> 
> Meanwhile, once spi_nor_remove() returns, devres_release_all(&spi->dev) frees
> nor->spimem, nor->dirmap, nor->params->vreg_offset, and nor->params->priv, and
> disables the vcc regulator:
> 
>    drivers/mtd/spi-nor/core.c:spi_nor_suspend() {
>        struct spi_nor *nor = mtd_to_spi_nor(mtd);
>        int ret;
> 
>        /* Disable octal DTR mode if we enabled it. */
>        ret = spi_nor_set_octal_dtr(nor, false);
>        ...
>    }
> 
>    drivers/mtd/spi-nor/core.c:spi_nor_resume() {
>        struct spi_nor *nor = mtd_to_spi_nor(mtd);
>        struct device *dev = nor->dev;
>        int ret;
> 
>        /* re-initialize the nor chip */
>        ret = spi_nor_init(nor);
>        ...
>    }
> 
> Since neither spi_nor_suspend() nor spi_nor_resume() acquires nor->lock or
> checks nor->removed, won't a suspend/resume cycle execute SPI operations
> through the freed nor->spimem and freed params fields?
> 

Agreed. Mainline already runs these on a freed spi_nor after unbind;
with this series the spi_nor stays allocated until the last close, so it
is narrower, but the suspend/resume paths still reach freed bus
resources. Needs the nor lock and a removed check in both. Left for a
follow-up.

>> -static int spi_nor_get_device(struct mtd_info *mtd)
>> +/* Free spi_nor and its buffers; safe on partial init. */
>> +static void spi_nor_free(struct spi_nor *nor)
>> +{
>> +	spi_nor_debugfs_unregister(nor);
>> +	kfree(nor->mtd.eraseregions);
>> +	kfree(nor->bouncebuf);
>> +	kfree(nor->params);
>> +	kfree(nor);
>> +}
>> +
>> +static void spi_nor_release(struct kref *kref)
>> +{
>> +	spi_nor_free(container_of(kref, struct spi_nor, refcnt));
>> +}
>> +
>> +static struct module *spi_nor_controller_owner(struct spi_nor *nor)
>>   {
>> -	struct mtd_info *master = mtd_get_master(mtd);
>> -	struct spi_nor *nor = mtd_to_spi_nor(master);
>>   	struct device *dev;
>>   
>> +	/* The controller may be gone on the last put; use the cached owner. */
>> +	if (nor->refcounted)
>> +		return nor->controller_module;
>> +
>>   	if (nor->spimem)
>>   		dev = nor->spimem->spi->controller->dev.parent;
>>   	else
>>   		dev = nor->dev;
>>   
>> -	if (!try_module_get(dev->driver->owner))
>> +	return dev->driver->owner;
>> +}
>> +
>> +static int spi_nor_get_device(struct mtd_info *mtd)
>> +{
>> +	struct mtd_info *master = mtd_get_master(mtd);
>> +	struct spi_nor *nor = mtd_to_spi_nor(master);
>> +
>> +	if (!try_module_get(spi_nor_controller_owner(nor)))
>>   		return -ENODEV;
>>   
>> +	if (nor->refcounted)
>> +		kref_get(&nor->refcnt);
>> +
>>   	return 0;
>>   }
> 
> [Severity: High]
> Should spi_nor_get_device() fail with -ENODEV once nor->removed is set?
> 
> When spi_nor_remove() runs while a handle is open on a partition (or master
> with CONFIG_MTD_PARTITIONED_MASTER), mtd_device_unregister() only decrements
> mtd->refcnt without calling mtd_device_release(), so the MTD device stays in
> mtd_idr until the open handle is closed and mtd_release() runs:
> 
>    mtdchar_open()
>      -> get_mtd_device()
>        -> __get_mtd_device()
>          -> spi_nor_get_device()
> 
> Without checking nor->removed (or using kref_get_unless_zero()), new opens of
> /dev/mtdX after unbind will still succeed and take new references on
> nor->refcnt, allowing userspace to keep struct spi_nor and the controller
> module pinned indefinitely after the original pre-unbind handle is closed.
> 
> In addition, in __put_mtd_device(), master->_put_device(master) ->
> spi_nor_put_device() frees struct spi_nor immediately when the last handle
> closes, whereas idr_remove(&mtd_idr, mtd->index) only runs in mtd_release()
> when all device references on mtd->dev drop. If anything holds a temporary
> reference on mtd->dev across the final close, could a concurrent
> get_mtd_device() still find the mtd in mtd_idr and call spi_nor_get_device()
> on a freed struct spi_nor?
> 
> [ ... ]
> 

Agreed. A new open after unbind succeeds and pins the object, and its
I/O then fails with -ENODEV. A removed check in ->_get_device() would
stop that; the lingering idr entry in the second paragraph is a core
lifetime question and would need a core change. Left for a follow-up.

>> @@ -3788,12 +3870,15 @@ static int spi_nor_probe(struct spi_mem *spimem)
>>   	if (ret)
>>   		return ret;
>>   
>> -	nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
>> +	nor = kzalloc_obj(*nor, GFP_KERNEL);
>>   	if (!nor)
>>   		return -ENOMEM;
>>   
>>   	nor->spimem = spimem;
>>   	nor->dev = dev;
>> +	nor->controller_module = spi->controller->dev.parent->driver->owner;
>> +	nor->refcounted = true;
>> +	kref_init(&nor->refcnt);
> 
> [Severity: High]
> Does nor->dev need a get_device() reference tied to nor->refcnt, or need to be
> avoided before nor->removed is checked?
> 
> With CONFIG_MTD_PARTITIONED_MASTER disabled, opening an unpartitioned master
> /dev/mtd0 calls __get_mtd_device(), which increments nor->refcnt via
> spi_nor_get_device() but does not increment master->refcnt:
> 
>    drivers/mtd/mtdcore.c:__get_mtd_device() {
>        ...
>        while (mtd) {
>            if (mtd != master)
>                kref_get(&mtd->refcnt);
>            mtd = mtd->parent;
>        }
> 
>        if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
>            kref_get(&master->refcnt);
>        ...
>    }
> 
> If the SPI controller is unbound or the SPI device is unregistered while
> /dev/mtd0 is open, spi_nor_remove() -> mtd_device_unregister() drops
> nor->mtd.refcnt to 0 and unregisters nor->mtd.dev immediately, releasing its
> parent reference on &spi->dev. Once spi_unregister_device() finishes, struct
> spi_device (nor->dev) is freed while struct spi_nor remains alive for the open
> descriptor.
> 
> A subsequent read, write, or erase on that open descriptor calls dev_dbg() on
> nor->dev before spi_nor_prep_and_lock*() checks nor->removed:
> 
>    drivers/mtd/spi-nor/core.c:spi_nor_read() {
>        ...
>        dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
> 
>        ret = spi_nor_prep_and_lock_rd(nor, from_lock, len_lock);
>        ...
>    }
> 
> The same pattern appears in spi_nor_write(), spi_nor_erase(), and
> sst_nor_write(). Could this dereference a freed nor->dev when debug logging is
> enabled?
> 

Agreed for unpartitioned flash without CONFIG_MTD_PARTITIONED_MASTER.
Holding a reference on nor->dev for the lifetime of spi_nor would fix
it. Left for a follow-up.

>>   	spi_nor_set_flash_node(nor, dev->of_node);
>>   
>>   	spi_mem_set_drvdata(spimem, nor);
> 
> [Severity: High]
> Right below this in spi_nor_probe(), nor->mtd.name is assigned from
> spi_mem_get_name(spimem):
> 
>    drivers/mtd/spi-nor/core.c:spi_nor_probe() {
>        ...
>        if (data && data->name)
>            nor->mtd.name = data->name;
> 
>        if (!nor->mtd.name)
>            nor->mtd.name = spi_mem_get_name(spimem);
>        ...
>    }
> 
> In spi_mem_probe(), spimem->name is either dev_name(&spi->dev) or a string
> allocated via ctlr->mem_ops->get_name(mem), which drivers such as
> spi-nxp-fspi, spi-fsl-qspi, spi-nxp-xspi, spi-aspeed-smc, and
> spi-cadence-quadspi allocate with devm_kasprintf().
> 
> When spi-nor or the SPI controller is unbound while an open handle keeps
> nor->mtd alive in mtd_idr and sysfs, devres_release_all() frees that name
> buffer. Won't subsequent reads of /proc/mtd (mtd_proc_show()),
> /sys/class/mtd/mtdX/name (mtd_name_show()), or lookups via
> get_mtd_device_nm() read freed memory from nor->mtd.name unless the string is
> duplicated and freed in spi_nor_free()?
> 
> [ ... ]
> 

Agreed. Duplicating the name at probe and freeing it with the spi_nor
would fix it. Left for a follow-up.

Happy to send these as a separate series if the maintainers prefer.

Thanks,
Tze Yee