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

[sashiko-bot@kernel.org]

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/