* Re: [PATCH RFC] clk: keystone: sci-clk: check the growing discovery array
2026-09-11 6:09 [PATCH RFC] clk: keystone: sci-clk: check the growing discovery array Slavin Liu
@ 2026-09-11 6:20 ` sashiko-bot
2026-09-11 19:36 ` Nishanth Menon
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-09-11 6:20 UTC (permalink / raw)
To: Slavin Liu; +Cc: linux-clk
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
^ permalink raw reply [flat|nested] 3+ messages in thread