Re: [PATCH v7 5/7] clk: nuvoton: ma35d1: Reparent SYSPLL correctly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] clk: buffer overflow via zero-length syspll_parent_data array
- [High] clk: unhandled ERR_PTR dereference in syspll_parent_data
--

commit 5e6e98e9249c8ddf8953662fc83c357de699a523
Author: Miquel Raynal <miquel.raynal@bootlin.com>

clk: nuvoton: ma35d1: Reparent SYSPLL correctly

The clock tree is still incorrect. SYSPLL is derived from HXT_GATE, very
much like CAPLL, DDRPLL, VPLL, APLL and EPLL. SYSPLL has a fixed
frequency, but if HXT gets gated, SYSPLL stops running.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
> index 2e6e1c65d01e5..0baaa93f13805 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
> @@ -73,6 +73,8 @@ static const struct clk_parent_data lxtg_parent_data[] = {
>  	{ .fw_name = "lxt", .name = "lxt" },
>  };
>  
> +static struct clk_parent_data syspll_parent_data[] = {};
> +

[Severity: Critical]
Does this code allocate a zero-length array in the .bss section?

Later in ma35d1_clocks_probe(), there is an unconditional write to
syspll_parent_data[0].hw, which seems to write entirely out-of-bounds
and corrupt 8 bytes of adjacent static memory.

[ ... ]

> @@ -275,22 +283,23 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>  
>  	/* Old DTs do not describe the low-speed crystal */
>  	if (num_clks == 1)
> -		hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
> +		hws[LXT] = ma35d1_clk_fixed(dev, "lxt", 32768);
>  
>  	hws[HXT_GATE] = ma35d1_clk_gate_parent_data(dev, "hxt_gate", hxtg_parent_data,
>  						    clk_base + REG_CLK_PWRCTL, 0);

[ ... ]

>  	hws[CAPLL] = ma35d1_reg_clk_pll(dev, CAPLL, pllmode[0], "capll",
>  					hws[HXT_GATE], clk_base + REG_CLK_PLL0CTL0);
> -	hws[SYSPLL] = ma35d1_clk_fixed("syspll", 180000000);
> +	syspll_parent_data[0].hw = hws[HXT_GATE];
> +	hws[SYSPLL] = ma35d1_clk_fixed_parent_data(dev, "syspll", 180000000, syspll_parent_data);

[Severity: High]
If ma35d1_clk_gate_parent_data() fails when allocating hws[HXT_GATE],
it returns an ERR_PTR. Could this ERR_PTR be blindly assigned to
syspll_parent_data[0].hw and registered via ma35d1_clk_fixed_parent_data()?

Without validating the pointer with IS_ERR(), the common clock framework
might dereference this ERR_PTR in clk_core_fill_parent_index() causing
a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-perso-ma35d1-upstream-clk-v7-0-cbd991fa6270@bootlin.com?part=5