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

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 <[email protected]>
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:

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

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