Re: [PATCH 1/2] xtensa/simdisk: Avoid referring to module::args

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent access to `dev->filename` in `proc_read_simdisk` and `simdisk_detach` causes a Use-After-Free.
--

commit e0a519c0b452f39fc91f2ac38f46b81a32db738a
Author: Petr Pavlu <petr.pavlu@suse.com>

xtensa/simdisk: Avoid referring to module::args

This commit prepares for the removal of module::args by copying the filename
string provided as a parameter instead of storing a direct pointer when built
as a loadable module. It also adds a free function to clean up these copied
strings upon module unload.

> diff --git a/arch/xtensa/platforms/iss/simdisk.c b/arch/xtensa/platforms/iss/simdisk.c
> index 7c7a2aa749f82..a2ddb49c269dc 100644
> --- a/arch/xtensa/platforms/iss/simdisk.c
> +++ b/arch/xtensa/platforms/iss/simdisk.c

[ ... ]

>  static const struct kernel_param_ops simdisk_param_ops_filename = {
>  	.set = simdisk_param_set_filename,
> +	.free = simdisk_param_free_filename,
>  };
> -module_param_cb(filename, &simdisk_param_ops_filename, &n_files, 0);
> +module_param_cb(filename, &simdisk_param_ops_filename, NULL, 0);
>  MODULE_PARM_DESC(filename, "Backing storage filename.");
>  
>  static int simdisk_major = SIMDISK_MAJOR;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible to trigger a
use-after-free here due to concurrent access to dev->filename?

The procfs entry for simdisk devices is created with 0644 permissions, allowing
any user to read it. In proc_read_simdisk(), dev->filename is fetched without
acquiring dev->lock:

arch/xtensa/platforms/iss/simdisk.c:proc_read_simdisk() {
    ...
    const char *s = dev->filename;
    if (s) {
        ssize_t len = strlen(s);
        char *temp = kmalloc(len + 2, GFP_KERNEL);
    ...
}

If an administrator concurrently writes to the same procfs entry, it triggers
proc_write_simdisk() -> simdisk_detach(), which acquires dev->lock and frees
the filename:

arch/xtensa/platforms/iss/simdisk.c:simdisk_detach() {
    ...
    spin_lock(&dev->lock);
    ...
        } else {
            pr_info("SIMDISK: %s detached from %s\n",
                    dev->gd->disk_name, dev->filename);
            dev->fd = -1;
            kfree(dev->filename);
            dev->filename = NULL;
        }
    ...
}

If proc_read_simdisk() fetches the pointer right before simdisk_detach() frees
it, the subsequent strlen() and scnprintf() operations could dereference freed
memory. Does this need locking or RCU protection to prevent a potential oops
or local information leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604110455.1608038-1-petr.pavlu@suse.com?part=1