[PATCH RFC] clk: keystone: sci-clk: check the growing discovery array

[PATCH RFC] clk: keystone: sci-clk: check the growing discovery array

[Slavin Liu]

Check each newly allocated discovery array before copying existing
entries or publishing its first element. Devres releases prior storage
when the probe fails.

Detected by static analysis and reviewed with AI-assisted source auditing.

Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
Assisted-by: LLM
Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
---
 drivers/clk/keystone/sci-clk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 9d2094bd48e3..dad2fb67c405 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -496,6 +496,8 @@ static int ti_sci_scan_clocks_from_fw(struct sci_clk_provider *provider)
 			tmp_clks = devm_kmalloc_array(dev, max_clks + 64,
 						      sizeof(sci_clk),
 						      GFP_KERNEL);
+			if (!tmp_clks)
+				return -ENOMEM;
 			memcpy(tmp_clks, clks, max_clks * sizeof(sci_clk));
 			if (max_clks)
 				devm_kfree(dev, clks);

Re: [PATCH RFC] clk: keystone: sci-clk: check the growing discovery array

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing of_node_put() leaks parsed device node references in args.np across normal iterations and error paths.
- [High] Calling memcpy() with a NULL pointer is undefined behavior when max_clks or num_clks is 0.
- [High] Firmware-provided num_parents exceeding 255 causes silent u8 integer truncation during clock registration.
--

commit 6d9b6324e89980ef5061c0575c46655eac6f8228
Author: Slavin Liu <bolin.liu@seu.edu.cn>

clk: keystone: sci-clk: check the growing discovery array

Check each newly allocated discovery array before copying existing
entries or publishing its first element. Devres releases prior storage
when the probe fails.

> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 9d2094bd48e3b..dad2fb67c4052 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -496,6 +496,8 @@ static int ti_sci_scan_clocks_from_fw(struct sci_clk_provider *provider)
>  			tmp_clks = devm_kmalloc_array(dev, max_clks + 64,
>  						      sizeof(sci_clk),
>  						      GFP_KERNEL);
> +			if (!tmp_clks)
> +				return -ENOMEM;
>  			memcpy(tmp_clks, clks, max_clks * sizeof(sci_clk));

[Severity: High]
This isn't a bug introduced by this patch, but when max_clks is 0 on the
first iteration, clks is NULL. Does this evaluate to memcpy(tmp_clks, NULL, 0)
and trigger undefined behavior?

Additionally, later in this function, if the firmware returned no clocks,
num_clks is 0 and clks is NULL. Does calling devm_kmemdup_array() ultimately
result in memcpy(ZERO_SIZE_PTR, NULL, 0) which is also undefined behavior?

>  			if (max_clks)
>  				devm_kfree(dev, clks);

Further down in the same ti_sci_scan_clocks_from_fw() function:

	sci_clk = devm_kzalloc(dev, sizeof(*sci_clk), GFP_KERNEL);
	if (!sci_clk)
		return -ENOMEM;
	sci_clk->dev_id = dev_id;
	sci_clk->clk_id = clk_id;
	sci_clk->provider = provider;
	sci_clk->num_parents = num_parents;

[Severity: High]
This is a pre-existing issue, but the num_parents fetched from firmware is a
32-bit value. Unlike the DT parsing path which explicitly limits it to 255,
there is no bounds checking here. 

When _sci_clk_build() later assigns this to init.num_parents (which is a u8
in struct clk_init_data), does a value >= 256 get silently truncated,
resulting in defective clock registration?


Also, looking at the related DT parsing path in ti_sci_scan_clocks_from_dt():

	do {
		ret = of_parse_phandle_with_args(np, *clk_name,
						 "#clock-cells", index,
						 &args);
		if (ret)
			break;

		if (args.args_count == 2 && args.np == dev->of_node) {
			sci_clk = devm_kzalloc(dev, sizeof(*sci_clk),
					       GFP_KERNEL);
			if (!sci_clk)
				return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this error path abort immediately
and leak both the previously acquired np and the newly acquired args.np?

[ ... ]
		index++;
	} while (args.np);

[Severity: High]
This is also a pre-existing issue, but does this loop finish using args.np
without ever calling of_node_put()? The reference appears to be overwritten
on the next iteration or lost when the loop terminates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911060906.94179-1-bolin.liu@seu.edu.cn?part=1

Re: [PATCH RFC] clk: keystone: sci-clk: check the growing discovery array

[Nishanth Menon]

On 14:09-20260911, Slavin Liu wrote:
> Check each newly allocated discovery array before copying existing
> entries or publishing its first element. Devres releases prior storage
> when the probe fails.
> 
> Detected by static analysis and reviewed with AI-assisted source auditing.

Why RFC?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource