Re: [PATCH] mtd: rawnand: gpmi: add debugfs entry for BCH geometry

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing cleanup of debugfs entries leads to a Use-After-Free on device unbind or module unload.
- [Medium] The use of a static global variable for the debugfs blob wrapper and a hardcoded directory name breaks support for multiple device instances.
- [Medium] The `raw_mode` debugfs file is created with NULL file operations, making it completely non-functional.
--

--- Patch [1]: [PATCH] mtd: rawnand: gpmi: add debugfs entry for BCH geometry ---
commit 687d6ba8c3ad7f3c6c8dbd852caa00f732194bfb
Author: Han Xu <[email protected]>

mtd: rawnand: gpmi: add debugfs entry for BCH geometry

Export the BCH geometry parameters via debugfs to aid debugging and
provide the necessary information for legacy kobs-ng tool. The debugfs
directory "gpmi-nand" is created under the root debugfs tree, exposing
the bch_geometry and raw_mode. The implementation is guarded with
#ifdef CONFIG_DEBUG_FS to avoid build failures.

> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index c1f766cb225aa..3ceb8df7f8f93 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c

[ ... ]

> @@ -732,6 +733,26 @@ static int common_nfc_set_geometry(struct gpmi_nand_data *this)
>  	return err;
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +static struct debugfs_blob_wrapper dbg_bch_geo;

[Severity: Medium]
Since this blob wrapper is declared as a static global variable, what happens
if there are multiple instances of the GPMI NAND controller probed?

It looks like the second instance will overwrite the global .data pointer
with its own geometry pointer, causing all reads from the debugfs file to
return the geometry of the most recently probed device.

> +static void bch_create_debugfs(struct gpmi_nand_data *this)
> +{
> +	struct bch_geometry *bch_geo = &this->bch_geometry;
> +	struct dentry *dbg_root;
> +
> +	dbg_root = debugfs_create_dir("gpmi-nand", NULL);

[Severity: High]
Does this code leak the debugfs directory and files when the driver is
unbound or the module is unloaded?

It appears that gpmi_nand_remove() lacks a corresponding call to
debugfs_remove() or debugfs_remove_recursive() to tear down this tree.
Since bch_geo points to memory within gpmi_nand_data which is managed
by devres, can this lead to a use-after-free if userspace reads the
debugfs file after the device is unbound?

Additionally, hardcoding the debugfs directory name to "gpmi-nand" might
cause creation conflicts if multiple controller instances are present.

> +	dbg_bch_geo.data = (void *)bch_geo;
> +	dbg_bch_geo.size = sizeof(struct bch_geometry);
> +	debugfs_create_blob("bch_geometry", 0444, dbg_root, &dbg_bch_geo);
> +	/* create raw mode flag */
> +	debugfs_create_file_full("raw_mode", 0444, dbg_root, NULL, NULL, NULL);

[Severity: Medium]
Are we passing NULL for the file operations here?

If the fops parameter is NULL, the debugfs core assigns
debugfs_noop_file_operations, which simply returns EOF (0 bytes) when
read. Does this make the raw_mode debugfs file completely non-functional
instead of exposing the intended boolean flag?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1