Re: [PATCH] clk: starfive: jh7110: reject oscillator lookup errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The clock notifier `jh7110_pll0_clk_notifier_cb` completely ignores the `ABORT_RATE_CHANGE` event, permanently leaving the CPU reparented to the slow oscillator if a clock rate change fails.
- [High] Missing cleanup in probe error paths causes a clock reference leak and a Use-After-Free (UAF) vulnerability in the notifier.
--

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

clk: starfive: jh7110: reject oscillator lookup errors

Clock lookup may fail while allocating a consumer reference. Return
a notifier error rather than passing ERR_PTR as the new clock parent.

> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> index 03c17cd2032f5..847c0a39e5ab5 100644
> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> @@ -390,6 +390,9 @@ static int jh7110_pll0_clk_notifier_cb(struct notifier_block *nb,
>  	if (action == PRE_RATE_CHANGE) {
>  		struct clk *osc = clk_get(priv->dev, "osc");
>  
> +		if (IS_ERR(osc))
> +			return notifier_from_errno(PTR_ERR(osc));
> +
>  		priv->original_clk = clk_get_parent(cpu_root);
>  		ret = clk_set_parent(cpu_root, osc);
>  		clk_put(osc);

[Severity: High]
This isn't a bug introduced by this patch, but does the notifier ignore the
ABORT_RATE_CHANGE event?

If a PLL rate change is aborted (e.g., rejected by a downstream clock
notifier returning NOTIFY_BAD, or failing at the hardware level), the
core clock framework emits an ABORT_RATE_CHANGE notification.

Because this event is ignored, the reparenting is never undone:

	} else if (action == POST_RATE_CHANGE) {
		ret = clk_set_parent(cpu_root, priv->original_clk);
	}

Could the CPU clock remain permanently parented to the 24MHz slow oscillator
instead of being restored, severely degrading system performance?


[Severity: High]
This is a pre-existing issue, but does jh7110_syscrg_probe() leave a dangling
notifier if a later registration fails?

If probe fails after successfully registering the clock notifier
(e.g., devm_clk_hw_register fails), the devres frees the priv structure.
However, the notifier is never unregistered:

drivers/clk/starfive/clk-starfive-jh7110-sys.c:jh7110_syscrg_probe() {
	...
	pllclk = clk_get(priv->dev, "pll0_out");
	if (IS_ERR(pllclk)) {
	...
	} else {
		priv->pll_clk_nb.notifier_call = jh7110_pll0_clk_notifier_cb;
		ret = clk_notifier_register(pllclk, &priv->pll_clk_nb);
		if (ret)
			return ret;
		priv->pll[0] = NULL;
	}
	...
		ret = devm_clk_hw_register(&pdev->dev, &clk->hw);
		if (ret)
			return ret; /* priv freed, but notifier still active */
	...
}

Could a subsequent rate change on pll0_out dereference the freed memory?
Also, does the pllclk reference obtained via clk_get() leak since it is
never freed with clk_put()?

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