[PATCH mtd] mtd: block2mtd: defer device open on module param write

[PATCH mtd] mtd: block2mtd: defer device open on module param write

[Adarsh Das]

When you write to /sys/module/block2mtd/parameters/block2mtd, the kernel
holds a module parameter lock for the whole callback. block2mtd was
opening the block device path inside that callback. Opening by path can
walk the filesystem and take overlayfs locks.

syzbot found this can create a circular lock dependency with overlayfs
(ovl_create_object). I was able to reproduce it with their C repro and
lockdep.

In this patch I applied the fix of copying the parameter string,
returning from the callback, then calling
block2mtd_setup2() from a work item so the device open happens after
the parameter lock is released.

Reported-by: syzbot <syzbot+7cab6a19619f1b8efc00@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?id=3e420300f6b231aa906425cbb4bd55dd64f42da4
Tested-by: Adarsh Das <adarshdas950@gmail.com>
Signed-off-by: Adarsh Das <adarshdas950@gmail.com>
---
 drivers/mtd/devices/block2mtd.c | 53 ++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 03e80b2c4f5a..35ce306ed8fb 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -18,6 +18,7 @@
 
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/workqueue.h>
 #include <linux/fs.h>
 #include <linux/blkdev.h>
 #include <linux/backing-dev.h>
@@ -33,6 +34,8 @@
 
 /* Maximum number of comma-separated items in the 'block2mtd=' parameter */
 #define BLOCK2MTD_PARAM_MAX_COUNT 3
+/* 80 for device, 12 for erase size, 80 for label, 8 for timeout */
+#define BLOCK2MTD_PARAM_MAXLEN (80 + 12 + 80 + 8)
 
 /* Info for the block device */
 struct block2mtd_dev {
@@ -405,8 +408,7 @@ static char block2mtd_paramline[80 + 12];
 
 static int block2mtd_setup2(const char *val)
 {
-	/* 80 for device, 12 for erase size, 80 for name, 8 for timeout */
-	char buf[80 + 12 + 80 + 8];
+	char buf[BLOCK2MTD_PARAM_MAXLEN];
 	char *str = buf;
 	char *token[BLOCK2MTD_PARAM_MAX_COUNT];
 	char *name;
@@ -461,11 +463,54 @@ static int block2mtd_setup2(const char *val)
 	return 0;
 }
 
+struct block2mtd_deferred {
+	struct work_struct work;
+	char *val;
+};
+
+static void block2mtd_deferred_workfn(struct work_struct *work)
+{
+	struct block2mtd_deferred *req =
+		container_of(work, struct block2mtd_deferred, work);
+
+	block2mtd_setup2(req->val);
+	kfree(req->val);
+	kfree(req);
+	module_put(THIS_MODULE);
+}
+
+static int block2mtd_defer_setup2(const char *val)
+{
+	struct block2mtd_deferred *req;
+
+	if (strnlen(val, BLOCK2MTD_PARAM_MAXLEN) >= BLOCK2MTD_PARAM_MAXLEN)
+		return -EINVAL;
+
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	req = kmalloc_obj(struct block2mtd_deferred, GFP_KERNEL);
+	if (!req) {
+		module_put(THIS_MODULE);
+		return -ENOMEM;
+	}
+
+	req->val = kstrdup(val, GFP_KERNEL);
+	if (!req->val) {
+		kfree(req);
+		module_put(THIS_MODULE);
+		return -ENOMEM;
+	}
+
+	INIT_WORK(&req->work, block2mtd_deferred_workfn);
+	schedule_work(&req->work);
+	return 0;
+}
 
 static int block2mtd_setup(const char *val, const struct kernel_param *kp)
 {
 #ifdef MODULE
-	return block2mtd_setup2(val);
+	return block2mtd_defer_setup2(val);
 #else
 	/* If more parameters are later passed in via
 	   /sys/module/block2mtd/parameters/block2mtd
@@ -473,7 +518,7 @@ static int block2mtd_setup(const char *val, const struct kernel_param *kp)
 	   we can parse the argument now. */
 
 	if (block2mtd_init_called)
-		return block2mtd_setup2(val);
+		return block2mtd_defer_setup2(val);
 
 	/* During early boot stage, we only save the parameters
 	   here. We must parse them later: if the param passed
-- 
2.55.0


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

Re: [PATCH mtd] mtd: block2mtd: defer device open on module param write

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mtd: block2mtd: list corruption due to missing lock in add_device()
- [High] mtd: block2mtd: async device creation breaks userspace ABI expectations
- [High] mtd: block2mtd: UAF on module unload due to unsafe module_put() in workqueue
- [High] mtd: block2mtd: UAF on module load failure due to uncancelled work
--

commit 2dff967915d7f4cb301503696152b0f9b535645f
Author: Adarsh Das <adarshdas950@gmail.com>

mtd: block2mtd: defer device open on module param write

This commit defers the block2mtd device setup to run asynchronously in a
workqueue. This avoids holding the module parameter lock while opening the
block device, which previously led to a circular lock dependency with
overlayfs.

Link: https://syzkaller.appspot.com/bug?id=3e420300f6b231aa906425cbb4bd55dd64f42da4

> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index 349fa07be314c..5ef2c15a6aa1a 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
[ ... ]
> @@ -461,11 +463,54 @@ static int block2mtd_setup2(const char *val)
>  	return 0;
>  }
>  
> +struct block2mtd_deferred {
> +	struct work_struct work;
> +	char *val;
> +};
> +
> +static void block2mtd_deferred_workfn(struct work_struct *work)
> +{
> +	struct block2mtd_deferred *req =
> +		container_of(work, struct block2mtd_deferred, work);
> +
> +	block2mtd_setup2(req->val);
> +	kfree(req->val);
> +	kfree(req);
> +	module_put(THIS_MODULE);

[Severity: High]
Can dropping the module reference here create a use-after-free during module
unload?

When the refcount drops to zero, rmmod proceeds and unmaps the module text.
The worker thread is still running inside the module's code space and will
jump to freed memory when returning from the function epilogue.

> +}
> +
> +static int block2mtd_defer_setup2(const char *val)
> +{
> +	struct block2mtd_deferred *req;
> +
> +	if (strnlen(val, BLOCK2MTD_PARAM_MAXLEN) >= BLOCK2MTD_PARAM_MAXLEN)
> +		return -EINVAL;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -ENODEV;
> +
> +	req = kmalloc_obj(struct block2mtd_deferred, GFP_KERNEL);
> +	if (!req) {
> +		module_put(THIS_MODULE);
> +		return -ENOMEM;
> +	}
> +
> +	req->val = kstrdup(val, GFP_KERNEL);
> +	if (!req->val) {
> +		kfree(req);
> +		module_put(THIS_MODULE);
> +		return -ENOMEM;
> +	}
> +
> +	INIT_WORK(&req->work, block2mtd_deferred_workfn);
> +	schedule_work(&req->work);

[Severity: High]
If module initialization fails after this point, is there a mechanism to
cancel this scheduled work?

For instance, if an invalid parameter is provided after a valid one during
module load (e.g., modprobe block2mtd block2mtd=/dev/loop0 invalid=1),
the module loader might free the module memory without waiting for the
work item. When the work executes, it would jump into unmapped memory.

[Severity: High]
Does this introduce a race condition with concurrent module parameter writes?

By deferring setup to a concurrent workqueue without adding synchronization,
multiple work items can execute add_device() concurrently on different CPUs
and race to modify the global blkmtd_device_list:

drivers/mtd/devices/block2mtd.c:add_device() {
    ...
    list_add(&dev->list, &blkmtd_device_list);
    ...
}

[Severity: High]
Does deferring the setup to a workqueue break userspace ABI expectations?

Before this patch, block2mtd_setup2() ran synchronously. Deferring it makes
the sysfs write return before the device is created, which might silently
break backwards compatibility for dependent userspace workflows.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915184659.6939-1-adarshdas950@gmail.com?part=1

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