Re: [PATCH 2/2] mtd: spi-nor: core: Fix use-after-free of spi_nor on unbind with open handles

["NG, TZE YEE" <tze.yee.ng@altera.com>]

On 21/9/2026 3:26 pm, Michael Walle wrote:
> On Thu Sep 3, 2026 at 5:14 AM CEST, tze.yee.ng wrote:
>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>
>> The spi_nor is allocated with devm_kzalloc() on the SPI device, so a
>> sysfs unbind frees it while /dev/mtdX is still open. A later close then
>> oopses in spi_nor_put_device()->module_put() on the freed spi_nor.
>> try_module_get() does not help: it blocks rmmod, not an unbind.
>>
>> Give the spi_nor (and its params and bouncebuf) a kref lifetime on the
>> spi-mem probe path so it survives until the last MTD user is gone, and
>> cache the controller module for the put path. The spimem and dirmaps
>> stay owned by the SPI core, so spi_nor_remove() drains in-flight
>> operations and sets nor->removed to fail later ones with -ENODEV.
>> Legacy controllers are unchanged.
>>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> 
> Sashiko had some remarks:
> 
> https://sashiko.dev/#/patchset/cover.1788404586.git.tze.yee.ng%40altera.com
> 
> Also how does the other mtd subsystems (spi-nand?) doing this? Do we
> have the wrong dev for devres?
> 
> -michael
> 

Thanks. I went through the Sashiko remarks.

The prep leak on signal and the legacy-controller path were
pre-existing, so I left them alone.

The rest were real holes in v1: eraseregions was still
devm-allocated, get_device() walked nor->spimem after unbind, and
suspend/resume had no removed check. Those convinced me the
spi-nor-local kref + nor->removed fence was the wrong place to fix
this. It duplicated mtd->refcnt and still left every other driver to
reimplement the same dance.

On the second question: I don't think we have the wrong device for 
devres. spi-nand does the same thing (devm on &mem->spi->dev). An open 
/dev/mtdX can outlive unbind, so anything allocated on the SPI device is 
already gone.

In v2, I will moves the lifetime into the MTD core: optional mtd->_free 
from the existing kref release, and mtd->removed so the core returns
-ENODEV for hw ops, new openers, and suspend/resume. spi-nor uses
that on the spi-mem path; legacy controllers are unchanged. spi-nand
is not converted in this series.

I'll send a v2 shortly.

Thanks,
Tze Yee

>> ---
>>   drivers/mtd/spi-nor/core.c  | 111 ++++++++++++++++++++++++++++++++----
>>   include/linux/mtd/spi-nor.h |  17 ++++++
>>   2 files changed, 118 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 8bc117b46e02..7fb61dde58f5 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -45,6 +45,9 @@
>>   #define SPI_NOR_SRST_SLEEP_MIN 200
>>   #define SPI_NOR_SRST_SLEEP_MAX 400
>>   
>> +static void spi_nor_unlock_and_unprep_pe(struct spi_nor *nor, loff_t start, size_t len);
>> +static void spi_nor_unlock_and_unprep_rd(struct spi_nor *nor, loff_t start, size_t len);
>> +
>>   /**
>>    * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
>>    *			   extension type.
>> @@ -1345,8 +1348,15 @@ int spi_nor_prep_and_lock(struct spi_nor *nor)
>>   	else
>>   		ret = wait_event_killable(nor->rww.wait,
>>   					  spi_nor_rww_start_exclusive(nor));
>> +	if (ret)
>> +		return ret;
>>   
>> -	return ret;
>> +	if (nor->removed) {
>> +		spi_nor_unlock_and_unprep(nor);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   void spi_nor_unlock_and_unprep(struct spi_nor *nor)
>> @@ -1416,8 +1426,15 @@ static int spi_nor_prep_and_lock_pe(struct spi_nor *nor, loff_t start, size_t le
>>   	else
>>   		ret = wait_event_killable(nor->rww.wait,
>>   					  spi_nor_rww_start_pe(nor, start, len));
>> +	if (ret)
>> +		return ret;
>>   
>> -	return ret;
>> +	if (nor->removed) {
>> +		spi_nor_unlock_and_unprep_pe(nor, start, len);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static void spi_nor_unlock_and_unprep_pe(struct spi_nor *nor, loff_t start, size_t len)
>> @@ -1489,8 +1506,15 @@ static int spi_nor_prep_and_lock_rd(struct spi_nor *nor, loff_t start, size_t le
>>   	else
>>   		ret = wait_event_killable(nor->rww.wait,
>>   					  spi_nor_rww_start_rd(nor, start, len));
>> +	if (ret)
>> +		return ret;
>>   
>> -	return ret;
>> +	if (nor->removed) {
>> +		spi_nor_unlock_and_unprep_rd(nor, start, len);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static void spi_nor_unlock_and_unprep_rd(struct spi_nor *nor, loff_t start, size_t len)
>> @@ -3186,7 +3210,12 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>   {
>>   	int ret;
>>   
>> -	nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params), GFP_KERNEL);
>> +	/* Keep params on the kref lifetime so it survives unbind (see probe). */
>> +	if (nor->refcounted)
>> +		nor->params = kzalloc(sizeof(*nor->params), GFP_KERNEL);
>> +	else
>> +		nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params),
>> +					   GFP_KERNEL);
>>   	if (!nor->params)
>>   		return -ENOMEM;
>>   
>> @@ -3420,6 +3449,22 @@ static void spi_nor_resume(struct mtd_info *mtd)
>>   		dev_err(dev, "resume() failed\n");
>>   }
>>   
>> +static void spi_nor_release(struct kref *kref)
>> +{
>> +	struct spi_nor *nor = container_of(kref, struct spi_nor, refcount);
>> +
>> +	kfree(nor->bouncebuf);
>> +	kfree(nor->params);
>> +	kfree(nor);
>> +}
>> +
>> +static void spi_nor_release_device(void *data)
>> +{
>> +	struct spi_nor *nor = data;
>> +
>> +	kref_put(&nor->refcount, spi_nor_release);
>> +}
>> +
>>   static int spi_nor_get_device(struct mtd_info *mtd)
>>   {
>>   	struct mtd_info *master = mtd_get_master(mtd);
>> @@ -3434,6 +3479,12 @@ static int spi_nor_get_device(struct mtd_info *mtd)
>>   	if (!try_module_get(dev->driver->owner))
>>   		return -ENODEV;
>>   
>> +	if (nor->refcounted) {
>> +		/* Cache the module: the spimem/controller chain may be freed by put time. */
>> +		nor->controller_module = dev->driver->owner;
>> +		kref_get(&nor->refcount);
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> @@ -3443,6 +3494,14 @@ static void spi_nor_put_device(struct mtd_info *mtd)
>>   	struct spi_nor *nor = mtd_to_spi_nor(master);
>>   	struct device *dev;
>>   
>> +	if (nor->refcounted) {
>> +		module_put(nor->controller_module);
>> +
>> +		/* Must be last: this may free nor (and the embedded mtd). */
>> +		kref_put(&nor->refcount, spi_nor_release);
>> +		return;
>> +	}
>> +
>>   	if (nor->spimem)
>>   		dev = nor->spimem->spi->controller->dev.parent;
>>   	else
>> @@ -3655,8 +3714,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>   	 * than 1KB) after spi_nor_scan() returns.
>>   	 */
>>   	nor->bouncebuf_size = PAGE_SIZE;
>> -	nor->bouncebuf = devm_kmalloc(dev, nor->bouncebuf_size,
>> -				      GFP_KERNEL);
>> +	if (nor->refcounted)
>> +		nor->bouncebuf = kmalloc(nor->bouncebuf_size, GFP_KERNEL);
>> +	else
>> +		nor->bouncebuf = devm_kmalloc(dev, nor->bouncebuf_size,
>> +					      GFP_KERNEL);
>>   	if (!nor->bouncebuf)
>>   		return -ENOMEM;
>>   
>> @@ -3788,10 +3850,21 @@ static int spi_nor_probe(struct spi_mem *spimem)
>>   	if (ret)
>>   		return ret;
>>   
>> -	nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
>> +	/*
>> +	 * An open /dev/mtdX handle can outlive unbind, so manage the spi_nor
>> +	 * with a kref and drop the probe-time reference from a devres callback.
>> +	 */
>> +	nor = kzalloc_obj(*nor, GFP_KERNEL);
>>   	if (!nor)
>>   		return -ENOMEM;
>>   
>> +	kref_init(&nor->refcount);
>> +	nor->refcounted = true;
>> +
>> +	ret = devm_add_action_or_reset(dev, spi_nor_release_device, nor);
>> +	if (ret)
>> +		return ret;
>> +
>>   	nor->spimem = spimem;
>>   	nor->dev = dev;
>>   	spi_nor_set_flash_node(nor, dev->of_node);
>> @@ -3830,9 +3903,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
>>   	 */
>>   	if (nor->params->page_size > PAGE_SIZE) {
>>   		nor->bouncebuf_size = nor->params->page_size;
>> -		devm_kfree(dev, nor->bouncebuf);
>> -		nor->bouncebuf = devm_kmalloc(dev, nor->bouncebuf_size,
>> -					      GFP_KERNEL);
>> +		kfree(nor->bouncebuf);
>> +		nor->bouncebuf = kmalloc(nor->bouncebuf_size, GFP_KERNEL);
>>   		if (!nor->bouncebuf)
>>   			return -ENOMEM;
>>   	}
>> @@ -3853,6 +3925,25 @@ static int spi_nor_remove(struct spi_mem *spimem)
>>   {
>>   	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>>   
>> +	/*
>> +	 * Drain in-flight operations and set nor->removed under the lock so
>> +	 * later ones fail with -ENODEV before touching SPI-core state (spimem,
>> +	 * dirmaps) freed after this returns. The wait is uninterruptible.
>> +	 */
>> +	if (!spi_nor_use_parallel_locking(nor))
>> +		mutex_lock(&nor->lock);
>> +	else
>> +		wait_event(nor->rww.wait, spi_nor_rww_start_exclusive(nor));
>> +
>> +	nor->removed = true;
>> +
>> +	if (!spi_nor_use_parallel_locking(nor)) {
>> +		mutex_unlock(&nor->lock);
>> +	} else {
>> +		spi_nor_rww_end_exclusive(nor);
>> +		wake_up(&nor->rww.wait);
>> +	}
>> +
>>   	spi_nor_restore(nor);
>>   
>>   	/* Clean up MTD stuff. */
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 4b92494827b1..f8d5e3ca8371 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -7,6 +7,7 @@
>>   #define __LINUX_MTD_SPI_NOR_H
>>   
>>   #include <linux/bitops.h>
>> +#include <linux/kref.h>
>>   #include <linux/mtd/mtd.h>
>>   #include <linux/spi/spi-mem.h>
>>   
>> @@ -352,6 +353,18 @@ struct spi_nor_flash_parameter;
>>    * @rww.used_banks:	bitmap of the banks in use
>>    * @dev:		pointer to an SPI device or an SPI NOR controller device
>>    * @spimem:		pointer to the SPI memory device
>> + * @refcount:		reference count keeping the kzalloc()'d spi_nor alive
>> + *			past driver unbind until the last MTD user releases the
>> + *			device. Only valid when @refcounted is set.
>> + * @controller_module:	controller module pinned in spi_nor_get_device() so
>> + *			spi_nor_put_device() need not walk the possibly freed
>> + *			spimem/controller chain. Only valid when @refcounted is
>> + *			set.
>> + * @refcounted:		true when the spi_nor lifetime is kref-managed (the
>> + *			spi-mem spi_nor_probe() path). Legacy controllers that
>> + *			embed or devres-allocate spi_nor leave this clear.
>> + * @removed:		set on unbind to make subsequent MTD operations fail
>> + *			with -ENODEV instead of touching released resources.
>>    * @bouncebuf:		bounce buffer used when the buffer passed by the MTD
>>    *                      layer is not DMA-able
>>    * @bouncebuf_size:	size of the bounce buffer
>> @@ -393,6 +406,10 @@ struct spi_nor {
>>   	} rww;
>>   	struct device		*dev;
>>   	struct spi_mem		*spimem;
>> +	struct kref		refcount;
>> +	struct module		*controller_module;
>> +	bool			refcounted;
>> +	bool			removed;
>>   	u8			*bouncebuf;
>>   	size_t			bouncebuf_size;
>>   	u8			*id;
>