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

[NG, TZE YEE]

Hi,

Gentle ping on this patch. Any chance for a review when you have time?

Thanks,
Tze Yee


On 23/10/2026 10:03 am, NG, TZE YEE wrote:
> 
> 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