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

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

[Aamir Ahmed]

Commit f316cdff8d67 ("clk: Annotate struct clk_hw_onecell_data with
__counted_by") annotated the hws member of 'struct clk_hw_onecell_data'
with __counted_by, which informs the bounds sanitizer (UBSAN_BOUNDS)
about the number of elements in .hws[], so that it can warn when .hws[]
is accessed out of bounds. As noted in that change, the __counted_by
member must be initialized with the number of elements before the first
array access happens, otherwise there will be a warning from each access
prior to the initialization because the number of elements is zero.
This occurs in ap_cpu_clock_probe(), which reads .hws[] to skip clusters
that have already been set up and writes it inside the CPU node loop,
but only assigns .num after the loop. With CONFIG_UBSAN_BOUNDS and a
compiler that implements __counted_by (GCC 15.1+ or Clang 20.1+), this
triggers an array-index-out-of-bounds report during probe of the AP806
and AP807 CPU clocks, and with CONFIG_UBSAN_TRAP the first access traps.

Initialize .num with nclusters, the number of elements .hws[] was
allocated with, right after the allocation. For the in-tree AP806 and
AP807 device trees, which list the CPU nodes in ascending order, this is
exactly the value the old assignment produced. It is also the correct
value for any other node order, since of_clk_hw_onecell_get() rejects
indices at or beyond .num.

Cc: stable@vger.kernel.org
Fixes: f316cdff8d67 ("clk: Annotate struct clk_hw_onecell_data with __counted_by")
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
Found while auditing the remaining clk_hw_onecell_data users that assign
.num only after touching .hws[], following the fixes already merged for
clk-s2mps11 (3e14c7207a97), exynos-clkout (cf33f0b7df13) and
clk-raspberrypi (6dc445c19050). The audit, the fix and this changelog
were drafted with an LLM assistant and reviewed by hand.

Compile-tested only (W=1, no warnings) on x86_64 with GCC 13.3, with
CONFIG_ARMADA_AP_CPU_CLK=y and CONFIG_ARMADA_AP_CP_HELPER=y forced on
the make command line because the driver has no COMPILE_TEST option. GCC
13.3 does not implement __counted_by (CC_HAS_COUNTED_BY needs GCC 15.1+
or Clang 20.1+), so the build only confirms that the change compiles;
the sanitizer path was not exercised. I do not have the hardware, so
this is not runtime-tested and no UBSAN report was captured.

Based on v7.3-rc1. On clk-next, the context of the first hunk differs by
one line (struct clk_init_data init = {};), so it needs a trivial
refresh or a three-way apply there.

 drivers/clk/mvebu/ap-cpu-clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/ap-cpu-clk.c b/drivers/clk/mvebu/ap-cpu-clk.c
index 1e44ace7d95..ed3ca59e0ee 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";
 		struct clk_init_data init;
@@ -333,8 +335,6 @@ static int ap_cpu_clock_probe(struct platform_device *pdev)
 		ap_cpu_data->hws[cluster_index] = &ap_cpu_clk[cluster_index].hw;
 	}
 
-	ap_cpu_data->num = cluster_index + 1;
-
 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, ap_cpu_data);
 	if (ret)
 		dev_err(dev, "failed to register OF clock provider\n");

base-commit: 654ae5d73c05bd2943d65636ce6cd0aa46e62f18
-- 
2.53.0.windows.1

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

[sashiko-bot]

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