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

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

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

An open /dev/mtdX can outlive driver unbind. The MTD core keeps
mtd_info registered until the last close, but a spi-nor allocated
with devm_*() on the SPI device is already freed, so a later
close/ioctl/read hits freed memory.

v1 tried to fix that with a spi-nor-local kref and nor->removed.
That duplicated mtd->refcnt and still missed get_device() after
unbind, suspend/resume, and eraseregions. v2 puts the lifetime in
the MTD core instead.

  - Patch 1: add optional mtd->_free() (called from kref release)
    and mtd->removed so the core returns -ENODEV for hw ops, new
    openers, and suspend/resume.
  - Patch 2: on the spi-mem path, allocate spi_nor / params /
    eraseregions / bouncebuf with kmalloc, install mtd->_free, and
    set mtd->removed in remove() after draining in-flight ops.
    Legacy controllers keep the old devres lifetime.

Changes in v2:
  - Drop the unrelated RWW self-deadlock fix.
  - New patch 1: MTD-core _free / removed hooks.
  - Patch 2: drop the spi-nor kref / refcounted / controller_module
    and the prep_and_lock*() removed checks; use the core hooks.
    Also keep eraseregions on that lifetime, free on the probe
    error path, and unregister debugfs from _free.

Tze Yee Ng (2):
  mtd: core: add release hook and removed flag for unbind with open
    handles
  mtd: spi-nor: fix use-after-free on unbind with open handles

 drivers/mtd/mtdcore.c         | 20 ++++++++
 drivers/mtd/spi-nor/core.c    | 92 +++++++++++++++++++++++++++++------
 drivers/mtd/spi-nor/core.h    |  2 +
 drivers/mtd/spi-nor/debugfs.c |  9 +---
 include/linux/mtd/mtd.h       | 13 +++++
 5 files changed, 112 insertions(+), 24 deletions(-)

-- 
2.43.7

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

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

An open /dev/mtdX can outlive unbind. refcnt defers device teardown, but
the driver-owned object embedding mtd_info (and its bus resources) may
already be gone when remove() returns — UAF for parent-devm allocations.

Add optional core hooks:

- mtd->_free(): called from kref release so the backing object lives until
  the last reference drops
- mtd->removed: set after draining ops; core returns -ENODEV for hw ops,
  new openers, and suspend/resume

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v2:
- New.
- Drop the spi-nor-local kref and nor->removed.
- Add mtd->_free and mtd->removed in the core so the backing
  object outlives unbind and later ops return -ENODEV.
---
 drivers/mtd/mtdcore.c   | 20 ++++++++++++++++++++
 include/linux/mtd/mtd.h | 13 +++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 491a27d0315f..59133894b5e5 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);
 }
 
 #define MTD_DEVICE_ATTR_RO(name) \
@@ -1332,6 +1336,9 @@ int __get_mtd_device(struct mtd_info *mtd)
 	struct mtd_info *master = mtd_get_master(mtd);
 	int err;
 
+	if (master->removed)
+		return -ENODEV;
+
 	if (master->_get_device) {
 		err = master->_get_device(master);
 		if (err)
@@ -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;
+
 	if (!mtd->erasesize || !master->_erase)
 		return -ENOTSUPP;
 
@@ -1792,6 +1802,9 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 
 	ops->retlen = ops->oobretlen = 0;
 
+	if (master->removed)
+		return -ENODEV;
+
 	ret_code = mtd_check_oob_ops(mtd, from, ops);
 	if (ret_code)
 		return ret_code;
@@ -1836,6 +1849,9 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
 
 	ops->retlen = ops->oobretlen = 0;
 
+	if (master->removed)
+		return -ENODEV;
+
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
 
@@ -2331,6 +2347,8 @@ int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *master = mtd_get_master(mtd);
 
+	if (master->removed)
+		return -ENODEV;
 	if (!master->_lock)
 		return -EOPNOTSUPP;
 	if (ofs < 0 || ofs >= mtd->size || len > mtd->size - ofs)
@@ -2351,6 +2369,8 @@ int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *master = mtd_get_master(mtd);
 
+	if (master->removed)
+		return -ENODEV;
 	if (!master->_unlock)
 		return -EOPNOTSUPP;
 	if (ofs < 0 || ofs >= mtd->size || len > mtd->size - ofs)
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 8d10d9d2e830..28fd98c4193c 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -362,6 +362,9 @@ struct mtd_info {
 	int (*_get_device) (struct mtd_info *mtd);
 	void (*_put_device) (struct mtd_info *mtd);
 
+	/* Optional free of the embedding object; called from kref release. */
+	void (*_free) (struct mtd_info *mtd);
+
 	/*
 	 * flag indicates a panic write, low level drivers can take appropriate
 	 * action if required to ensure writes go through
@@ -380,6 +383,10 @@ struct mtd_info {
 	struct module *owner;
 	struct device dev;
 	struct kref refcnt;
+
+	/* Set on unbind while refs held; core returns -ENODEV for hw ops. */
+	bool removed;
+
 	struct mtd_debug_info dbg;
 	struct nvmem_device *nvmem;
 	struct nvmem_device *otp_user_nvmem;
@@ -548,6 +555,9 @@ static inline int mtd_suspend(struct mtd_info *mtd)
 	struct mtd_info *master = mtd_get_master(mtd);
 	int ret;
 
+	if (master->removed)
+		return 0;
+
 	if (master->master.suspended)
 		return 0;
 
@@ -563,6 +573,9 @@ static inline void mtd_resume(struct mtd_info *mtd)
 {
 	struct mtd_info *master = mtd_get_master(mtd);
 
+	if (master->removed)
+		return;
+
 	if (!master->master.suspended)
 		return;
 
-- 
2.43.7

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

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

spi_nor and its params, erase-region table and bounce buffer are
devm-allocated on the SPI device, so unbind frees them while /dev/mtdX
is still open. The MTD core keeps mtd_info around until the last close,
but the backing spi_nor is already gone, so a later close/ioctl/read
hits freed memory.

On the spi-mem probe path, allocate them with plain kmalloc/kzalloc and
install mtd->_free so the core frees them when the last handle closes.
In remove(), drain in-flight ops and set mtd->removed so later ops
return -ENODEV before touching the spimem/dirmaps freed by the SPI core.

Legacy controllers keep the devres lifetime (no mtd->_free) and are
unchanged.

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v2:
- Drop the spi-nor kref, nor->refcounted, and controller_module.
  Use mtd->_free / mtd->removed from patch 1.
- Drop the nor->removed checks in prep_and_lock*(). The core now
  fences hw ops, new openers, and suspend/resume.
- Keep eraseregions on the same lifetime as params/bouncebuf
  (plain kcalloc when mtd->_free is set).
- Free params/bouncebuf/eraseregions/nor on the probe error path.
  Unregister debugfs from _free instead of a devm action.
- Legacy controllers still use devm (no mtd->_free).
---
 drivers/mtd/spi-nor/core.c    | 92 +++++++++++++++++++++++++++++------
 drivers/mtd/spi-nor/core.h    |  2 +
 drivers/mtd/spi-nor/debugfs.c |  9 +---
 3 files changed, 79 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 8bc117b46e02..b28d896f16ed 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3145,6 +3145,11 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
 	}
 }
 
+static bool spi_nor_uses_free_hook(struct spi_nor *nor)
+{
+	return nor->mtd._free;
+}
+
 /**
  * spi_nor_init_params() - Initialize the flash's parameters and settings.
  * @nor:	pointer to a 'struct spi_nor'.
@@ -3186,7 +3191,11 @@ static int spi_nor_init_params(struct spi_nor *nor)
 {
 	int ret;
 
-	nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params), GFP_KERNEL);
+	if (spi_nor_uses_free_hook(nor))
+		nor->params = kzalloc_obj(*nor->params, GFP_KERNEL);
+	else
+		nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params),
+					   GFP_KERNEL);
 	if (!nor->params)
 		return -ENOMEM;
 
@@ -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;
 
@@ -3655,8 +3668,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 (spi_nor_uses_free_hook(nor))
+		nor->bouncebuf = kmalloc(nor->bouncebuf_size, GFP_KERNEL);
+	else
+		nor->bouncebuf = devm_kmalloc(dev, nor->bouncebuf_size,
+					      GFP_KERNEL);
 	if (!nor->bouncebuf)
 		return -ENOMEM;
 
@@ -3770,6 +3786,22 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor)
 	return PTR_ERR_OR_ZERO(nor->dirmap.wdesc);
 }
 
+/* Free spi_nor and related buffers; safe on partial init (kfree(NULL)). */
+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);
+}
+
+/* MTD core ->_free callback: last reference dropped, release the backing. */
+static void spi_nor_mtd_free(struct mtd_info *mtd)
+{
+	spi_nor_free(mtd_to_spi_nor(mtd));
+}
+
 static int spi_nor_probe(struct spi_mem *spimem)
 {
 	struct spi_device *spi = spimem->spi;
@@ -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;
 	spi_nor_set_flash_node(nor, dev->of_node);
 
 	spi_mem_set_drvdata(spimem, nor);
@@ -3819,7 +3852,7 @@ static int spi_nor_probe(struct spi_mem *spimem)
 
 	ret = spi_nor_scan(nor, flash_name, &hwcaps);
 	if (ret)
-		return ret;
+		goto err_free;
 
 	spi_nor_debugfs_register(nor);
 
@@ -3830,29 +3863,56 @@ 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);
-		if (!nor->bouncebuf)
-			return -ENOMEM;
+		kfree(nor->bouncebuf);
+		nor->bouncebuf = kmalloc(nor->bouncebuf_size, GFP_KERNEL);
+		if (!nor->bouncebuf) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
 	}
 
 	ret = spi_nor_create_read_dirmap(nor);
 	if (ret)
-		return ret;
+		goto err_free;
 
 	ret = spi_nor_create_write_dirmap(nor);
 	if (ret)
-		return ret;
+		goto err_free;
 
-	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;
 }
 
 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);
+	}
+
 	spi_nor_restore(nor);
 
 	/* Clean up MTD stuff. */
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index ba2d1a862c9d..e71b788b275c 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -726,9 +726,11 @@ bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr
 
 #ifdef CONFIG_DEBUG_FS
 void spi_nor_debugfs_register(struct spi_nor *nor);
+void spi_nor_debugfs_unregister(struct spi_nor *nor);
 void spi_nor_debugfs_shutdown(void);
 #else
 static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
+static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
 static inline void spi_nor_debugfs_shutdown(void) {}
 #endif
 
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index 288e2866daed..a0639e91cb2d 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -286,10 +286,8 @@ static int spi_nor_capabilities_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
 
-static void spi_nor_debugfs_unregister(void *data)
+void spi_nor_debugfs_unregister(struct spi_nor *nor)
 {
-	struct spi_nor *nor = data;
-
 	debugfs_remove(nor->debugfs_root);
 	nor->debugfs_root = NULL;
 }
@@ -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;
 
-- 
2.43.7

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

[sashiko-bot]

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@altera.com>

    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?


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


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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791512623.git.tze.yee.ng@altera.com?part=1

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

[sashiko-bot]

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.

[ ... ]
> @@ -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.

[ ... ]
> @@ -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.

[ ... ]
> @@ -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.

>  
>  	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.

> 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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791512623.git.tze.yee.ng@altera.com?part=2