Re: [PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

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

Pre-existing issues:
- [High] Exposing the `measure_summary` debugfs file before initializing the `priv` pointers in the `msr_table` leads to a potential NULL pointer dereference.
--

commit cf66aebca628dec91393028bc843130a21da66ad
Author: Pengpeng Hou <[email protected]>
Subject: soc: amlogic: meson-clk-measure: remove debugfs tree

This commit adds a remove callback to the driver to properly clean up the
debugfs tree created during probe. By storing the debugfs root and removing
it on driver unbind, it prevents leftover entries from referencing freed
private data.

> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
> index d862e30a244ed..7ca43bcb622a2 100644
> --- a/drivers/soc/amlogic/meson-clk-measure.c
> +++ b/drivers/soc/amlogic/meson-clk-measure.c
[ ... ]
> @@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
>  	       sizeof(struct msr_reg_offset));
>  
>  	root = debugfs_create_dir("meson-clk-msr", NULL);
> +	priv->debugfs_root = root;
>  	clks = debugfs_create_dir("clks", root);
>  
>  	debugfs_create_file("measure_summary", 0444, root,
>  			    priv->data.msr_table, &clk_msr_summary_fops);
>  
>  	for (i = 0 ; i < priv->data.msr_count ; ++i) {
>  		if (!priv->data.msr_table[i].name)
>  			continue;
>  
>  		priv->data.msr_table[i].priv = priv;

[Severity: High]
This is a pre-existing issue, but does creating the measure_summary debugfs
file here expose it to userspace before the priv pointers in msr_table are
initialized?

If a user-space process reads measure_summary concurrently while the loop
below is still executing, clk_msr_summary_show() could dereference a NULL
pointer via msr_table->priv:

clk_msr_summary_show() {
    struct meson_msr_id *msr_table = s->private;
    unsigned int msr_count = msr_table->priv->data.msr_count;
    ...
}

Could the measure_summary debugfs file be created after the priv fields are
fully initialized in the loop to avoid this race?

>  		debugfs_create_file(priv->data.msr_table[i].name, 0444, clks,
>  				    &priv->data.msr_table[i], &clk_msr_fops);
>  	}
>  
> +	platform_set_drvdata(pdev, priv);
> +
>  	return 0;
>  }

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic