Re: [PATCH] soc: amlogic: clk-measure: Mark the clock measure table as __initconst

[Jian Hu <jian.hu@amlogic.com>]

Hi Neil,


On 7/6/2026 1:43 PM, sashiko-bot@kernel.org wrote:
> [ EXTERNAL EMAIL ]
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] Marking `clk_msr_*` arrays as `__initconst` introduces a section mismatch and a use-after-free vulnerability.
>
> Pre-existing issues:
> - [High] The driver lacks a `.remove` callback to clean up debugfs files, leading to a use-after-free when the device is unbound.
> - [Medium] `clk_msr_summary_show` blindly assumes that the first entry (index 0) in the clock measure table is populated, which will cause a null pointer dereference if a table lacks an entry for ID 0.
> --
>
> commit a817a04ade20a00516808ca084a60df1513d5b20
> Author: Jian Hu <jian.hu@amlogic.com>
> soc: amlogic: clk-measure: Mark the clock measure table as __initconst
>
> The clock measure table is only used during initialization, and the
> table data is copied to the msr_table field of struct meson_msr_data.
> Mark it as __initconst so it can be freed after kernel initialization.
>
>> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
>> index 8c4f3cc8c8abf..e1cf67fad83cb 100644
>> --- a/drivers/soc/amlogic/meson-clk-measure.c
>> +++ b/drivers/soc/amlogic/meson-clk-measure.c
>> @@ -55,7 +55,7 @@ struct meson_msr {
>>   #define CLK_MSR_ID(__id, __name) \
>>        [__id] = {.id = __id, .name = __name,}
>>
>> -static const struct meson_msr_id clk_msr_m8[] = {
>> +static const struct meson_msr_id clk_msr_m8[] __initconst = {
> [Severity: High]
> Does marking this array as __initconst cause a use-after-free and a section
> mismatch?
>
> Because this driver is a regular platform_driver, it can be probed after boot
> due to deferred probing or manual sysfs bind. If probed after initialization,
> the memcpy() in meson_msr_probe() will read from freed init memory:


Please ignore this patch.

After taking another look, I realized that marking the data with 
__initconst is not correct,
since it is still referenced after the init section is discarded.

>
>          memcpy(priv->data.msr_table, match_data->msr_table,
>                 match_data->msr_count * sizeof(struct meson_msr_id));
>
> Additionally, this array is referenced by clk_msr_m8_data:
>
> static const struct meson_msr_data clk_msr_m8_data = {
>          .msr_table = (void *)clk_msr_m8,
> ...
>
> Since clk_msr_m8_data resides in .rodata, wouldn't pointing it to __initconst
> memory cause a linker section mismatch warning?
>
> [ ... ]
>
> [Severity: High]
> This is a pre-existing issue, but does this driver lack a .remove callback to
> clean up debugfs files, leading to a use-after-free on device unbind?
>
> In meson_msr_probe(), debugfs files are created and associated with memory
> allocated via devm_kcalloc():
>
>          root = debugfs_create_dir("meson-clk-msr", NULL);
>          clks = debugfs_create_dir("clks", root);
>
>          debugfs_create_file("measure_summary", 0444, root,
>                              priv->data.msr_table, &clk_msr_summary_fops);
>
> The driver lacks a .remove callback and does not use
> devm_add_action_or_reset() to clean up the debugfs directory:
>
> static struct platform_driver meson_msr_driver = {
>          .probe  = meson_msr_probe,
>          .driver = {
> ...
>
> If the device is unbound via sysfs, devres will automatically free
> priv->data.msr_table. Any subsequent read from the persistent debugfs files
> will dereference the freed memory. Should a cleanup mechanism be added?
>
> [Severity: Medium]
> This is a pre-existing issue, but does clk_msr_summary_show() blindly assume
> the first entry in the clock measure table is populated?
>
> Looking at clk_msr_summary_show():
>
>          struct meson_msr_id *msr_table = s->private;
>          unsigned int msr_count = msr_table->priv->data.msr_count;
>
> This immediately dereferences msr_table->priv. In meson_msr_probe(), the priv
> pointer is only assigned if the clock ID is defined (i.e., name is non-NULL).
> If a future SoC table were to omit an entry for ID 0, msr_table[0].priv would
> be NULL, causing a kernel panic here. Could this be made more robust against
> empty first entries?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260706-clkmsr_optimization-v1-1-70f5aae6e60b@amlogic.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic