Re: [PATCH 4/5] rtc: philips: Make sure clk_init_data is fully initialized

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unmanaged `of_clk_add_provider()` causes memory leak and Use-After-Free (UAF) on probe failure or driver unbind.
--

--- Patch [4]: [PATCH 4/5] rtc: philips: Make sure clk_init_data is fully initialized ---
commit 33e5514e66c5385440eb77c7db4a09f5f2b1c50d
Author: Geert Uytterhoeven <geert+renesas@glider.be>

rtc: philips: Make sure clk_init_data is fully initialized

This commit zero-initializes clk_init_data upon declaration to prevent subtle
bugs from uninitialized mutually-exclusive members. This helps avoid issues
exposed when certain stack initialization configurations are enabled.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 01e209d88f5f3..8cb9ffc73f6d9 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -498,7 +498,7 @@ static const struct clk_ops pcf85063_clkout_ops = {
>  static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
>  {
>  	struct clk *clk;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free and
resource leak regarding the clock provider registered later in this function.

In pcf85063_clkout_register_clk(), the clock provider is added using the
unmanaged API without any cleanup mechanism:

	clk = devm_clk_register(&pcf85063->rtc->dev, &pcf85063->clkout_hw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

If the probe later fails when calling devm_rtc_register_device() in
pcf85063_probe():

	return devm_rtc_register_device(pcf85063->rtc);

Or if the driver is unbound via sysfs, devres will automatically free the
clock structure and driver data.

Since of_clk_add_provider() is unmanaged, does this leave the clock provider
in the global of_clk_providers list pointing to a freed clock?

It seems a subsequent device tree lookup could invoke of_clk_src_simple_get()
and cause a use-after-free by returning the dangling pointer.

Would it be better to use a managed API or devm_add_action_or_reset()
to ensure the provider is safely removed on teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787241693.git.geert+renesas@glider.be?part=4