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

[PATCH v3 0/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>

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 or
mtdblock flush uses freed memory.

v2 moved the lifetime into the MTD core (mtd->_free and mtd->removed).
Review pointed out that this creates new problems in the core: the master
can be freed inside __put_mtd_device() while it is still used, the driver
object leaks when CONFIG_MTD_PARTITIONED_MASTER is off, and a lockless
removed check races with remove(). Only spi-nor needs the fix, so v3
goes back to a spi-nor-local kref and reduces the core change to one
reorder:
  - Patch 1: in __put_mtd_device(), call ->_put_device() first and drop the
    module reference last, so the owner module stays pinned while
    ->_put_device() frees the driver object.
  - Patch 2: on the spi-mem path, allocate spi_nor, params, erase regions
    and the bounce buffer with kmalloc and refcount spi_nor (one
    reference for registration plus one per open handle). remove()
    unregisters debugfs and the MTD device, then sets nor->removed under
    the nor lock; prep_and_lock*() re-check it under the lock and return
    -ENODEV. Legacy controllers keep the devres lifetime.

Not covered:
  - A sysfs file held open across unbind is a core lifetime question for
    all MTD drivers and is left out of this series.

---
Changes in v3:
  - Drop the core _free/removed hooks. Patch 1 is only the module_put()
    reorder.
  - Patch 2: spi-nor kref again, removed re-checked under the lock,
    controller module cached at probe, debugfs and MTD unregistered at the
    start of remove() so mtdblock can flush, eraseregions leak and probe
    error paths fixed.

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.

v2: https://lore.kernel.org/all/cover.1791512623.git.tze.yee.ng@altera.com/

Tze Yee Ng (2):
  mtd: core: drop the module reference after ->_put_device()
  mtd: spi-nor: fix use-after-free on unbind with open handles

 drivers/mtd/mtdcore.c         |   9 +-
 drivers/mtd/spi-nor/core.c    | 173 +++++++++++++++++++++++++++-------
 drivers/mtd/spi-nor/core.h    |   2 +
 drivers/mtd/spi-nor/debugfs.c |   9 +-
 include/linux/mtd/spi-nor.h   |   9 ++
 5 files changed, 159 insertions(+), 43 deletions(-)

-- 
2.43.7


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v3 1/2] mtd: core: drop the module reference after ->_put_device()

[tze.yee.ng]

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

__put_mtd_device() calls module_put() on the MTD owner and then calls
->_put_device(). If ->_put_device() frees the object that embeds the
mtd_info, it is still running code from the owner module after the
module reference is gone, so the module can be unloaded underneath it.

Call ->_put_device() first and module_put() last. ->_put_device() may
free master, so read master->owner beforehand.

The spi-nor fix that follows needs this: its ->_put_device() frees
struct spi_nor on the last put.

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v3:
- Reduced to the module_put() reorder. The core _free/removed hooks from
  v2 are gone; spi-nor manages its own lifetime again (patch 2).
- __put_mtd_device() now reads master->owner first, calls ->_put_device()
  and drops the module reference last (v2 left master->owner and
  ->_put_device() reachable after the core could free master).

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 491a27d0315f..1cf402e07b13 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1435,6 +1435,8 @@ EXPORT_SYMBOL_GPL(put_mtd_device);
 void __put_mtd_device(struct mtd_info *mtd)
 {
 	struct mtd_info *master = mtd_get_master(mtd);
+	/* master can be freed in _put_device */
+	struct module *owner = master->owner;
 
 	while (mtd) {
 		/* kref_put() can relese mtd, so keep a reference mtd->parent */
@@ -1448,11 +1450,12 @@ void __put_mtd_device(struct mtd_info *mtd)
 	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 */
+	/* must be the last use of master */
 	if (master->_put_device)
 		master->_put_device(master);
+
+	/* _put_device() runs code from @owner, so drop the module last */
+	module_put(owner);
 }
 EXPORT_SYMBOL_GPL(__put_mtd_device);
 
-- 
2.43.7


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v3 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>

Unbind frees spi_nor and its buffers (devm) while /dev/mtdX may still be
open, so a later read, ioctl, close or mtdblock flush uses freed memory.

On the spi-mem probe path, allocate them with kmalloc/kzalloc and
refcount spi_nor: one reference for registration plus one per open
handle. The last put frees them.

In remove(), unregister debugfs (it reads devres data) and the MTD
device first, so mtdblock can still flush. Then set nor->removed under
the nor lock; prep_and_lock*() return -ENODEV once it is set. Cache the
controller module at probe, as ->_put_device() now runs after unbind.

Legacy controllers keep devres (nor->refcounted is unset).

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v3:
- Use a spi-nor kref again (nor->refcnt, nor->refcounted, nor->removed,
  nor->controller_module) and drop the core hooks from v2. Patch 1 is now
  only the module_put() reorder it needs.
- Re-check nor->removed under nor->lock (or the RWW exclusive section) in
  prep_and_lock*(), instead of a lockless check in the core. A check done
  before taking the lock can pass just before remove() and then operate on
  freed dirmaps.
- Cache the controller module at probe: ->_put_device() runs after unbind
  and dereferenced the freed nor->spimem and a NULL dev->driver.
- In remove(), unregister debugfs first (it reads devres data), then call
  mtd_device_unregister() before fencing, so that the mtdblock release
  notifier can still flush its dirty cache. Then set nor->removed, restore
  and drop the registration reference. The debugfs devres action is gone.
- Assign mtd->eraseregions right after allocation so the error path
  in spi_nor_set_mtd_eraseregions() doesn't leak it.
- Free through the kref on probe errors. On a failed mtd_device_register(),
  fence ops first, as a handle may already be open.
- Move the unlock_and_unprep_pe/rd() prototypes to the top of the file.

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    | 173 +++++++++++++++++++++++++++-------
 drivers/mtd/spi-nor/core.h    |   2 +
 drivers/mtd/spi-nor/debugfs.c |   9 +-
 include/linux/mtd/spi-nor.h   |   9 ++
 4 files changed, 153 insertions(+), 40 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 8bc117b46e02..76928bd8413e 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,16 @@ 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;
+	/* remove() sets @removed under the lock, so this check is race-free. */
+	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 +1427,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 +1507,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 +3211,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 (nor->refcounted)
+		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;
 
@@ -3420,20 +3449,48 @@ static void spi_nor_resume(struct mtd_info *mtd)
 		dev_err(dev, "resume() failed\n");
 }
 
-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;
 }
 
@@ -3441,14 +3498,12 @@ static void spi_nor_put_device(struct mtd_info *mtd)
 {
 	struct mtd_info *master = mtd_get_master(mtd);
 	struct spi_nor *nor = mtd_to_spi_nor(master);
-	struct device *dev;
 
-	if (nor->spimem)
-		dev = nor->spimem->spi->controller->dev.parent;
-	else
-		dev = nor->dev;
+	module_put(spi_nor_controller_owner(nor));
 
-	module_put(dev->driver->owner);
+	/* May free @nor. */
+	if (nor->refcounted)
+		kref_put(&nor->refcnt, spi_nor_release);
 }
 
 static void spi_nor_restore(struct spi_nor *nor)
@@ -3549,11 +3604,18 @@ 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 (nor->refcounted)
+		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;
 
+	/* Let spi_nor_free() release it on the error path below. */
+	mtd->eraseregions = mtd_region;
+
 	for (i = 0; i < map->n_regions; i++) {
 		erasesize = spi_nor_get_region_erasesize(&region[i],
 							 map->erase_type);
@@ -3566,7 +3628,6 @@ static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
 	}
 
 	mtd->numeraseregions = map->n_regions;
-	mtd->eraseregions = mtd_region;
 
 	return 0;
 }
@@ -3655,8 +3716,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;
 
@@ -3770,6 +3834,24 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor)
 	return PTR_ERR_OR_ZERO(nor->dirmap.wdesc);
 }
 
+/* Drain in-flight ops, then fence later ones with -ENODEV. */
+static void spi_nor_set_removed(struct spi_nor *nor)
+{
+	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);
+	}
+}
+
 static int spi_nor_probe(struct spi_mem *spimem)
 {
 	struct spi_device *spi = spimem->spi;
@@ -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);
 	spi_nor_set_flash_node(nor, dev->of_node);
 
 	spi_mem_set_drvdata(spimem, nor);
@@ -3819,7 +3904,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,33 +3915,57 @@ 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) {
+		/* A handle may be open on a partition that was already visible. */
+		spi_nor_set_removed(nor);
+		goto err_free;
+	}
+
+	return 0;
+
+err_free:
+	spi_nor_debugfs_unregister(nor);
+	kref_put(&nor->refcnt, spi_nor_release);
+	return ret;
 }
 
 static int spi_nor_remove(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+	int ret;
+
+	/* debugfs reads devres data (nor->id, erase map) freed after remove() */
+	spi_nor_debugfs_unregister(nor);
+
+	/* Unregister first: notifiers (e.g. mtdblock) flush dirty data here. */
+	ret = mtd_device_unregister(&nor->mtd);
+
+	spi_nor_set_removed(nor);
 
 	spi_nor_restore(nor);
 
-	/* Clean up MTD stuff. */
-	return mtd_device_unregister(&nor->mtd);
+	/* Drop the registration reference; frees @nor unless handles are open. */
+	kref_put(&nor->refcnt, spi_nor_release);
+
+	return ret;
 }
 
 static void spi_nor_shutdown(struct spi_mem *spimem)
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;
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4b92494827b1..b67d720e809d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -352,6 +352,11 @@ 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
+ * @refcnt:		registration + open handles; the last put frees @nor.
+ *			Only used when @refcounted is set.
+ * @refcounted:		@nor is not devres-managed (spi-mem probe path)
+ * @removed:		set under @lock on unbind; later operations fail
+ * @controller_module:	controller driver module, cached at probe
  * @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 +398,10 @@ struct spi_nor {
 	} rww;
 	struct device		*dev;
 	struct spi_mem		*spimem;
+	struct kref		refcnt;
+	bool			refcounted;
+	bool			removed;
+	struct module		*controller_module;
 	u8			*bouncebuf;
 	size_t			bouncebuf_size;
 	u8			*id;
-- 
2.43.7


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 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 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@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?

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

[ ... ]

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

>  	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()?

[ ... ]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [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@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@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



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/