[PATCH] mtd: slram: avoid dangling device list entries

[PATCH] mtd: slram: avoid dangling device list entries

[Ruoyu Wang]

register_device() links a new slram_mtdlist entry before the entry is
fully initialized. If a later allocation, memremap(), or
mtd_device_register() fails, the failed entry can remain reachable from
the global list and later cleanup can dereference or free invalid state.

Build the new entry off-list, unwind partial initialization locally on
failure, and only publish the entry after mtd_device_register()
succeeds.

Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/mtd/devices/slram.c | 95 ++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 44 deletions(-)

diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
index 69cb63d99f573..c7f6beba078fe 100644
--- a/drivers/mtd/devices/slram.c
+++ b/drivers/mtd/devices/slram.c
@@ -129,71 +129,78 @@ static int slram_write(struct mtd_info *mtd, loff_t to, size_t len,
 static int register_device(char *name, unsigned long start, unsigned long length)
 {
 	slram_mtd_list_t **curmtd;
+	slram_mtd_list_t *new_mtd;
+	struct mtd_info *mtdinfo = NULL;
+	slram_priv_t *priv = NULL;
+	int ret = -ENOMEM;
 
 	curmtd = &slram_mtdlist;
 	while (*curmtd) {
 		curmtd = &(*curmtd)->next;
 	}
 
-	*curmtd = kmalloc_obj(slram_mtd_list_t);
-	if (!(*curmtd)) {
+	new_mtd = kmalloc_obj(slram_mtd_list_t);
+	if (!new_mtd) {
 		E("slram: Cannot allocate new MTD device.\n");
-		return(-ENOMEM);
+		return -ENOMEM;
 	}
-	(*curmtd)->mtdinfo = kzalloc_obj(struct mtd_info);
-	(*curmtd)->next = NULL;
+	new_mtd->next = NULL;
 
-	if ((*curmtd)->mtdinfo)	{
-		(*curmtd)->mtdinfo->priv =
-			kzalloc_obj(slram_priv_t);
-
-		if (!(*curmtd)->mtdinfo->priv) {
-			kfree((*curmtd)->mtdinfo);
-			(*curmtd)->mtdinfo = NULL;
-		}
+	mtdinfo = kzalloc_obj(struct mtd_info);
+	if (!mtdinfo) {
+		E("slram: Cannot allocate new MTD device.\n");
+		goto out_free;
 	}
+	new_mtd->mtdinfo = mtdinfo;
 
-	if (!(*curmtd)->mtdinfo) {
+	priv = kzalloc_obj(slram_priv_t);
+	if (!priv) {
 		E("slram: Cannot allocate new MTD device.\n");
-		return(-ENOMEM);
+		goto out_free;
 	}
+	mtdinfo->priv = priv;
 
-	if (!(((slram_priv_t *)(*curmtd)->mtdinfo->priv)->start =
-		memremap(start, length,
-			 MEMREMAP_WB | MEMREMAP_WT | MEMREMAP_WC))) {
+	priv->start = memremap(start, length,
+			       MEMREMAP_WB | MEMREMAP_WT | MEMREMAP_WC);
+	if (!priv->start) {
 		E("slram: memremap failed\n");
-		return -EIO;
+		ret = -EIO;
+		goto out_free;
 	}
-	((slram_priv_t *)(*curmtd)->mtdinfo->priv)->end =
-		((slram_priv_t *)(*curmtd)->mtdinfo->priv)->start + length;
-
-
-	(*curmtd)->mtdinfo->name = name;
-	(*curmtd)->mtdinfo->size = length;
-	(*curmtd)->mtdinfo->flags = MTD_CAP_RAM;
-	(*curmtd)->mtdinfo->_erase = slram_erase;
-	(*curmtd)->mtdinfo->_point = slram_point;
-	(*curmtd)->mtdinfo->_unpoint = slram_unpoint;
-	(*curmtd)->mtdinfo->_read = slram_read;
-	(*curmtd)->mtdinfo->_write = slram_write;
-	(*curmtd)->mtdinfo->owner = THIS_MODULE;
-	(*curmtd)->mtdinfo->type = MTD_RAM;
-	(*curmtd)->mtdinfo->erasesize = SLRAM_BLK_SZ;
-	(*curmtd)->mtdinfo->writesize = 1;
-
-	if (mtd_device_register((*curmtd)->mtdinfo, NULL, 0))	{
+	priv->end = priv->start + length;
+
+	mtdinfo->name = name;
+	mtdinfo->size = length;
+	mtdinfo->flags = MTD_CAP_RAM;
+	mtdinfo->_erase = slram_erase;
+	mtdinfo->_point = slram_point;
+	mtdinfo->_unpoint = slram_unpoint;
+	mtdinfo->_read = slram_read;
+	mtdinfo->_write = slram_write;
+	mtdinfo->owner = THIS_MODULE;
+	mtdinfo->type = MTD_RAM;
+	mtdinfo->erasesize = SLRAM_BLK_SZ;
+	mtdinfo->writesize = 1;
+
+	ret = mtd_device_register(mtdinfo, NULL, 0);
+	if (ret) {
 		E("slram: Failed to register new device\n");
-		memunmap(((slram_priv_t *)(*curmtd)->mtdinfo->priv)->start);
-		kfree((*curmtd)->mtdinfo->priv);
-		kfree((*curmtd)->mtdinfo);
-		return(-EAGAIN);
+		goto out_unmap;
 	}
+	*curmtd = new_mtd;
 	T("slram: Registered device %s from %luKiB to %luKiB\n", name,
 			(start / 1024), ((start + length) / 1024));
 	T("slram: Mapped from 0x%p to 0x%p\n",
-			((slram_priv_t *)(*curmtd)->mtdinfo->priv)->start,
-			((slram_priv_t *)(*curmtd)->mtdinfo->priv)->end);
-	return(0);
+			priv->start, priv->end);
+	return 0;
+
+out_unmap:
+	memunmap(priv->start);
+out_free:
+	kfree(priv);
+	kfree(mtdinfo);
+	kfree(new_mtd);
+	return ret;
 }
 
 static void unregister_devices(void)
-- 
2.51.0


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

Re: [PATCH] mtd: slram: avoid dangling device list entries

[Miquel Raynal]

Hello Ruoyu,

On 09/06/2026 at 00:29:39 +08, Ruoyu Wang <ruoyuw560@gmail.com> wrote:

> register_device() links a new slram_mtdlist entry before the entry is
> fully initialized. If a later allocation, memremap(), or
> mtd_device_register() fails, the failed entry can remain reachable from
> the global list and later cleanup can dereference or free invalid state.
>
> Build the new entry off-list, unwind partial initialization locally on
> failure, and only publish the entry after mtd_device_register()
> succeeds.
>
> Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>

I am sorry but I cannot take this patch as-is because it does way more
than what you claim. It makes a lot of cleanup changes which are totally
unrelated (yet welcome). Please create a series with:
- the fix (that can be backported) first, with Cc: stable and Fixes: tags
- all the cleanups you want to do after.

Thanks,
Miquèl

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