[PATCH 0/2] mtd: spi-nor: Fix unbind deadlock and use-after-free

[PATCH 0/2] mtd: spi-nor: Fix unbind deadlock and use-after-free

[tze.yee.ng]

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

Two fixes in the spi-nor core around locking and driver unbind:

Patch 1 fixes a self-deadlock in spi_nor_rww_start_exclusive(), which
took nor->lock while its end helper re-acquired it via guard(mutex).

Patch 2 fixes a use-after-free where a sysfs unbind frees the
devm-allocated spi_nor while /dev/mtdX is still open. Give it a kref
lifetime on the spi-mem probe path and fail operations with -ENODEV
after unbind. Legacy controllers are unchanged.

Tze Yee Ng (2):
  mtd: spi-nor: Fix deadlock in spi_nor_rww_start_exclusive()
  mtd: spi-nor: core: Fix use-after-free of spi_nor on unbind with open
    handles

 drivers/mtd/spi-nor/core.c  | 113 ++++++++++++++++++++++++++++++++----
 include/linux/mtd/spi-nor.h |  17 ++++++
 2 files changed, 119 insertions(+), 11 deletions(-)

-- 
2.43.7

[PATCH 1/2] mtd: spi-nor: Fix deadlock in spi_nor_rww_start_exclusive()

[tze.yee.ng]

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

spi_nor_rww_start_exclusive() takes nor->lock with a bare mutex_lock()
and returns with it held, while its counterpart
spi_nor_rww_end_exclusive() re-acquires nor->lock via guard(mutex). Any
exclusive operation on a read-while-write (SNOR_F_RWW) part thus
deadlocks against itself.

The exclusive section is serialized by the rww ongoing_io/rd/pe flags,
so take nor->lock with guard(mutex) like the sibling start helpers.

Fixes: 03e7bb864d9a9 ("mtd: spi-nor: use scope-based mutex cleanup helpers")
Cc: stable@vger.kernel.org
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ccf4396cdcd0..8bc117b46e02 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1310,7 +1310,7 @@ static bool spi_nor_rww_start_exclusive(struct spi_nor *nor)
 {
 	struct spi_nor_rww *rww = &nor->rww;
 
-	mutex_lock(&nor->lock);
+	guard(mutex)(&nor->lock);
 
 	if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
 		return false;
-- 
2.43.7

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

[tze.yee.ng]

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>
---
 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;
-- 
2.43.7