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 <[email protected]> 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/[email protected]?part=1
