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

[sashiko-bot@kernel.org]

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