Re: [PATCH] clk: mvebu: ap-cpu-clk: Assign .num before accessing .hws

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver unconditionally triggers a kernel panic or memory corruption by mutating a read-only string literal (.rodata).
- [High] Clock initialization applies uninitialized stack garbage as configuration flags.
- [Medium] The parent clock reference is permanently leaked upon every iteration.
--

commit b7cdd9438cc10aafbe039089be5e257e331a9472
Author: Aamir Ahmed <elb12345@hotmail.co.uk>

clk: mvebu: ap-cpu-clk: Assign .num before accessing .hws

This commit initializes the num member of struct clk_hw_onecell_data before
the first array access to the hws member. This fixes an out-of-bounds warning
triggered by the bounds sanitizer when accessing the array elements.

> diff --git a/drivers/clk/mvebu/ap-cpu-clk.c b/drivers/clk/mvebu/ap-cpu-clk.c
> index 1e44ace7d9519..ed3ca59e0ee97 100644
> --- a/drivers/clk/mvebu/ap-cpu-clk.c
> +++ b/drivers/clk/mvebu/ap-cpu-clk.c
> @@ -284,6 +284,8 @@ static int ap_cpu_clock_probe(struct platform_device *pdev)
>  	if (!ap_cpu_data)
>  		return -ENOMEM;
>  
> +	ap_cpu_data->num = nclusters;
> +
>  	for_each_of_cpu_node(dn) {
>  		char *clk_name = "cpu-cluster-0";
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but can this unconditionally trigger a kernel
panic or memory corruption?

By declaring clk_name as a pointer to a string literal in read-only memory
(.rodata), the driver attempts an illegal write operation when it executes:

ap_cpu_clock_probe() {
    ...
    clk_name[12] += cluster_index;
    ...
}

Could this memory protection fault be prevented by declaring clk_name as a
character array instead of a pointer?

>  		struct clk_init_data init;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does this allocation apply uninitialized
stack garbage as configuration flags?

When struct clk_init_data init is allocated on the stack without being
zero-initialized, the clock framework might read uninitialized stack
memory and apply arbitrary flags (like CLK_IGNORE_UNUSED or
CLK_IS_CRITICAL) when passed to devm_clk_hw_register().

Could init be zero-initialized to avoid misinterpreting uninitialized fields?

[Severity: Medium]
This is a pre-existing issue, but is the parent clock reference permanently
leaked upon every iteration?

When retrieving the parent clock, of_clk_get() acquires a reference:

ap_cpu_clock_probe() {
    ...
    parent = of_clk_get(np, cluster_index);
    if (IS_ERR(parent)) {
        dev_err(dev, "Could not get the clock parent\n");
        of_node_put(dn);
        return -EINVAL;
    }
    parent_name =  __clk_get_name(parent);
    ...
}

Is it necessary to call clk_put() on the parent clock before the loop
advances or the function returns, to prevent consuming memory and elevating
the clock's reference count permanently?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/AS8P251MB000199E5C86A0447B3FC48EDC8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM?part=1