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

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

Hi maintainers,

Thanks. Going through these, here is what I plan for v3:

On 9/10/2026 3:57 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [Critical] mtd: spi-nor: double-free on mtd_device_register error path
> - [High] mtd: spi-nor: UAF via TOCTOU race from lockless master->removed check
> - [High] mtd: spi-nor: UAF on nor->id and erase_map via debugfs after unbind
> - [High] mtd: spi-nor: UAF and NULL pointer deref in spi_nor_put_device() on unbind
> - [Medium] mtd: spi-nor: memory leak of mtd_region on error path
> - [Medium] mtd: spi-nor: memory leak of spi_nor object on unbind
> --
> 
> commit cf3ecfea956c974ea676c9cd71ff2115f0af32ca
> Author: Tze Yee Ng <tze.yee.ng@altera.com>
> 
> mtd: spi-nor: fix use-after-free on unbind with open handles
> 
> The commit switches spi_nor objects from devm_kzalloc to kzalloc with
> a custom mtd->_free hook to ensure they outlive the device if file handles
> are held open. It also sets a removed flag in the remove callback to
> fence off hardware operations during devres teardown.
> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 8bc117b46e028..b28d896f16ed5 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
> [ ... ]
>> @@ -3549,8 +3558,12 @@ static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
>>   	struct mtd_info *mtd = &nor->mtd;
>>   	u32 erasesize, i;
>>   
>> -	mtd_region = devm_kcalloc(nor->dev, map->n_regions, sizeof(*mtd_region),
>> -				  GFP_KERNEL);
>> +	if (spi_nor_uses_free_hook(nor))
>> +		mtd_region = kcalloc(map->n_regions, sizeof(*mtd_region),
>> +				     GFP_KERNEL);
>> +	else
>> +		mtd_region = devm_kcalloc(nor->dev, map->n_regions,
>> +					  sizeof(*mtd_region), GFP_KERNEL);
>>   	if (!mtd_region)
>>   		return -ENOMEM;
> 
> [Severity: Medium]
> Since mtd_region is now allocated with kcalloc, does it get leaked if
> spi_nor_get_region_erasesize() fails later in this function?
> 
> If an early return happens here:
> 
> drivers/mtd/spi-nor/core.c:spi_nor_set_mtd_eraseregions() {
>      ...
>      for (i = 0; i < map->n_regions; i++) {
>          erasesize = spi_nor_get_region_erasesize(&region[i],
>                                                   map->erase_type);
>          if (!erasesize)
>              return -EINVAL;
>      ...
> }
> 
> mtd_region isn't yet assigned to mtd->eraseregions, so spi_nor_free() will
> just call kfree(NULL) and leak this allocation.
> 
> [ ... ]

Agreed. I plan to assign mtd->eraseregions right after allocation, so
the error path frees it.

>> @@ -3788,12 +3820,13 @@ 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->mtd._free = spi_nor_mtd_free;
> 
> [Severity: High]
> Does extending the lifetime of the spi_nor object cause a crash in
> spi_nor_put_device() if the file is closed after the driver unbinds?
> 
> When the MTD device is eventually closed, mtdchar_close() calls
> spi_nor_put_device(). Since nor is still alive, it will attempt to access
> nor->spimem to find the controller. However, nor->spimem was allocated
> via devres and freed on unbind. This also attempts to access dev->driver
> which will be NULL.
> 

Agreed. In v3, the controller module will be cached at probe and used in
->_get_device()/->_put_device() instead of nor->spimem and dev->driver. 
Patch 1 will keeps the module pinned until ->_put_device() returns.

> [ ... ]
>> @@ -3830,29 +3863,56 @@ static int spi_nor_probe(struct spi_mem *spimem)
> [ ... ]
>> -	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>> -				   data ? data->nr_parts : 0);
>> +	ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>> +				  data ? data->nr_parts : 0);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	spi_nor_free(nor);
>> +	return ret;
>>   }
> 
> [Severity: Critical]
> Can this cause a double-free of the spi_nor object?
> 
> If mtd_device_register() succeeds in registering the master MTD but fails
> subsequently (e.g., partition allocation fails), the MTD core's error cleanup
> drops the master's kref to 0. This triggers mtd->_free (spi_nor_free) and
> frees the object.
> 
> Then mtd_device_register() returns an error code, causing a jump to err_free
> where spi_nor_free(nor) is called a second time on the already-freed pointer.
> 

Agreed. I will fix in v3. With no core _free hook, the core would never 
free the spi_nor. On a failed mtd_device_register() the probe would 
fence ops and drop the single registration reference, so the object is 
released exactly once.

> [ ... ]
>> @@ -3875,6 +3908,21 @@ static int spi_nor_remove(struct spi_mem *spimem)
>>   {
>>   	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>>   
>> +	/*
>> +	 * Drain in-flight ops and mark removed so later ops get -ENODEV
>> +	 * before touching bus resources freed after this returns.
>> +	 */
>> +	if (!spi_nor_use_parallel_locking(nor))
>> +		mutex_lock(&nor->lock);
>> +	else
>> +		wait_event(nor->rww.wait, spi_nor_rww_start_exclusive(nor));
>> +
>> +	nor->mtd.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);
>> +	}
> 
> [Severity: High]
> Does this create a TOCTOU race with lockless readers?
> 
> A reader in the MTD core can check master->removed locklessly and proceed.
> 
> If that reader blocks waiting for nor->lock, and spi_nor_remove() concurrently
> acquires the lock, sets nor->mtd.removed = true, releases the lock, and returns,
> the driver unbind completes and devres frees resources like nor->spimem.
> 
> The blocked reader will then wake up, acquire the lock (failing to check
> removed again), and try to access the freed devres resources.
> 

I will fix in v3. removed is set under nor->lock (or the RWW exclusive
section) and re-checked under the lock in prep_and_lock(), _pe() and
_rd().

>>   
>>   	spi_nor_restore(nor);
>>   
>>   	/* Clean up MTD stuff. */
>>   	return mtd_device_unregister(&nor->mtd);
>>   }
> 
> [Severity: Medium]
> Will this leak the entire spi_nor object if the device has partitions but
> CONFIG_MTD_PARTITIONED_MASTER is disabled?
> 
> When that config is disabled, the master MTD is never registered. In that
> scenario, mtd_device_unregister() returns 0 early without invoking kref_put()
> on the master.
> 
> This means mtd->_free() will never be called, permanently leaking the
> kzalloc'd spi_nor object.
> 

Agreed, same cause as the core leak above. remove() would always drop 
the registration reference, whatever mtd_device_unregister() returns.

>> diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
>> index 288e2866daedb..a0639e91cb2d9 100644
>> --- a/drivers/mtd/spi-nor/debugfs.c
>> +++ b/drivers/mtd/spi-nor/debugfs.c
> [ ... ]
>> @@ -299,15 +297,10 @@ static struct dentry *rootdir;
>>   void spi_nor_debugfs_register(struct spi_nor *nor)
>>   {
>>   	struct dentry *d;
>> -	int ret;
>>   
>>   	if (!rootdir)
>>   		rootdir = debugfs_create_dir(SPI_NOR_DEBUGFS_ROOT, NULL);
>>   
>> -	ret = devm_add_action(nor->dev, spi_nor_debugfs_unregister, nor);
>> -	if (ret)
>> -		return;
>> -
>>   	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
>>   	nor->debugfs_root = d;
> 
> [Severity: High]
> Since this devm_add_action() is removed, do the debugfs files remain
> accessible after unbind completes, while we wait for the final mtd close?
> 
> If the debugfs files persist, accessing them can cause a use-after-free.
> 
> For example, reading the params file will call spi_nor_params_show(), which
> dereferences nor->id and erase_map.regions. Since those were allocated via
> devres, they are freed when the driver unbinds, even if the spi_nor object
> itself is kept alive.
> 

Agreed. In v3, spi_nor_debugfs_unregister() will be called at the start 
of remove(), before devres frees nor->id and the erase map, and again on 
the probe error path.

I'll send v3 with these changes.

Thanks,
Tze Yee