Re: [PATCH v9 01/10] clk: nuvoton: ma35d1: Use clk_hw pointers as mux parents

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: ma35d1: unmanaged mux clocks leak and cause UAF on probe failure
--

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

clk: nuvoton: ma35d1: Use clk_hw pointers as mux parents

This commit updates the MA35D1 clock provider to use actual hardware clocks
as mux parents rather than relying on parent data structures. This resolves
an issue where parent lookups failed, leading to an incorrect, flat clock
tree and missing rate assignments.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
> --- a/drivers/clk/nuvoton/clk-ma35d1.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
[ ... ]
>  static struct clk_hw *ma35d1_clk_mux(struct device *dev, const char *name,
>  				     void __iomem *reg, u8 shift, u8 width,
> -				     const struct clk_parent_data *pdata,
> -				     int num_pdata)
> +				     struct clk_hw **hws,
> +				     const int *parent_idx, int num_parents)
>  {
> -	return clk_hw_register_mux_parent_data(dev, name, pdata, num_pdata,
> -					       CLK_SET_RATE_NO_REPARENT, reg, shift,
> -					       width, 0, &ma35d1_lock);
> +	const struct clk_hw *parent_hws[MA35D1_MUX_MAX_PARENTS] = {};
> +	int i;
> +
> +	for (i = 0; i < num_parents; i++)
> +		parent_hws[i] = (parent_idx[i] >= 0) ? hws[parent_idx[i]] : NULL;
> +
> +	return clk_hw_register_mux_hws(dev, name, parent_hws, num_parents,
> +				       CLK_SET_RATE_NO_REPARENT, reg, shift,
> +				       width, 0, &ma35d1_lock);
>  }

[Severity: High]
If device probe fails after these muxes are registered (for example, if
devm_of_clk_add_hw_provider() returns -ENOMEM near the end of
ma35d1_clocks_probe()), the devres subsystem will unwind managed allocations,
unmap clk_base, and free ma35d1_hw_data.

Since the mux clocks registered here using clk_hw_register_mux_hws() are
unmanaged, won't they remain registered in the clock core and permanently
leak their internal struct clk_mux allocations?

Furthermore, because they remain registered, could subsequent clock
interactions dereference the now-unmapped clk_base register pointers,
causing a use-after-free or unmapped memory access?

While a later patch in this series addresses a similar issue for fixed clocks
by switching to a devm_ variant, ma35d1_clk_mux() still uses the unmanaged
clk_hw_register_mux_hws(). Should this be updated to use the appropriate
managed devm_ variant to prevent this regression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-perso-ma35d1-upstream-clk-v9-0-1f548fa2002e@bootlin.com?part=1