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

On 6/15/26 11:24, [email protected] wrote:
> 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?

Impossible the driver is initialized way before userspace starts.

> 
>>   		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;
>>   }
> 


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