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

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

[Jian Hu via B4 Relay]

From: Jian Hu <jian.hu@amlogic.com>

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.
Taking T7 as an example, this frees approximately 5 KB of memory,
saving some runtime memory.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
 drivers/soc/amlogic/meson-clk-measure.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
index 8c4f3cc8c8ab..e1cf67fad83c 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 = {
 	CLK_MSR_ID(0, "ring_osc_out_ee0"),
 	CLK_MSR_ID(1, "ring_osc_out_ee1"),
 	CLK_MSR_ID(2, "ring_osc_out_ee2"),
@@ -104,7 +104,7 @@ static const struct meson_msr_id clk_msr_m8[] = {
 	CLK_MSR_ID(63, "mipi_csi_cfg"),
 };
 
-static const struct meson_msr_id clk_msr_gx[] = {
+static const struct meson_msr_id clk_msr_gx[] __initconst = {
 	CLK_MSR_ID(0, "ring_osc_out_ee_0"),
 	CLK_MSR_ID(1, "ring_osc_out_ee_1"),
 	CLK_MSR_ID(2, "ring_osc_out_ee_2"),
@@ -174,7 +174,7 @@ static const struct meson_msr_id clk_msr_gx[] = {
 	CLK_MSR_ID(82, "ge2d"),
 };
 
-static const struct meson_msr_id clk_msr_axg[] = {
+static const struct meson_msr_id clk_msr_axg[] __initconst = {
 	CLK_MSR_ID(0, "ring_osc_out_ee_0"),
 	CLK_MSR_ID(1, "ring_osc_out_ee_1"),
 	CLK_MSR_ID(2, "ring_osc_out_ee_2"),
@@ -248,7 +248,7 @@ static const struct meson_msr_id clk_msr_axg[] = {
 	CLK_MSR_ID(109, "audio_locker_in"),
 };
 
-static const struct meson_msr_id clk_msr_g12a[] = {
+static const struct meson_msr_id clk_msr_g12a[] __initconst = {
 	CLK_MSR_ID(0, "ring_osc_out_ee_0"),
 	CLK_MSR_ID(1, "ring_osc_out_ee_1"),
 	CLK_MSR_ID(2, "ring_osc_out_ee_2"),
@@ -364,7 +364,7 @@ static const struct meson_msr_id clk_msr_g12a[] = {
 	CLK_MSR_ID(122, "audio_pdm_dclk"),
 };
 
-static const struct meson_msr_id clk_msr_sm1[] = {
+static const struct meson_msr_id clk_msr_sm1[] __initconst = {
 	CLK_MSR_ID(0, "ring_osc_out_ee_0"),
 	CLK_MSR_ID(1, "ring_osc_out_ee_1"),
 	CLK_MSR_ID(2, "ring_osc_out_ee_2"),
@@ -494,7 +494,7 @@ static const struct meson_msr_id clk_msr_sm1[] = {
 	CLK_MSR_ID(127, "csi2_data"),
 };
 
-static const struct meson_msr_id clk_msr_c3[] = {
+static const struct meson_msr_id clk_msr_c3[] __initconst = {
 	CLK_MSR_ID(0,	"sys_clk"),
 	CLK_MSR_ID(1,	"axi_clk"),
 	CLK_MSR_ID(2,	"rtc_clk"),
@@ -634,7 +634,7 @@ static const struct meson_msr_id clk_msr_c3[] = {
 
 };
 
-static const struct meson_msr_id clk_msr_s4[] = {
+static const struct meson_msr_id clk_msr_s4[] __initconst = {
 	CLK_MSR_ID(0, "sys_clk"),
 	CLK_MSR_ID(1, "axi_clk"),
 	CLK_MSR_ID(2, "rtc_clk"),
@@ -787,7 +787,7 @@ static const struct meson_msr_id clk_msr_s4[] = {
 
 };
 
-static const struct meson_msr_id clk_msr_a1[] = {
+static const struct meson_msr_id clk_msr_a1[] __initconst = {
 	CLK_MSR_ID(0, "tdmout_b_sclk"),
 	CLK_MSR_ID(1, "tdmout_a_sclk"),
 	CLK_MSR_ID(2, "tdmin_lb_sclk"),
@@ -843,7 +843,7 @@ static const struct meson_msr_id clk_msr_a1[] = {
 	CLK_MSR_ID(61, "osc_ring_ramb"),
 };
 
-static const struct meson_msr_id clk_msr_t7[] = {
+static const struct meson_msr_id clk_msr_t7[] __initconst = {
 	CLK_MSR_ID(0, "sys"),
 	CLK_MSR_ID(1, "axi"),
 	CLK_MSR_ID(2, "rtc"),

---
base-commit: 4336e970ec6890fbd424128c352564a9c1dd514a
change-id: 20260706-clkmsr_optimization-b8e1cd70bfee

Best regards,
-- 
Jian Hu <jian.hu@amlogic.com>



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

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

[sashiko-bot]

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:

	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

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

[Jian Hu]

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