Re: [PATCH v4 06/11] rtc: sun6i: add Allwinner B288 compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: sun6i: missing ERR_PTR check causes crash in probe on clk_register failure
- [Medium] rtc: sun6i: resource leak of losc clock on initialization error path
- [Low] rtc: sun6i: memory leak of rtc structure in sun6i_rtc_clk_init error path
--

commit 903d00f6b5cb6afede9ad6ad5fca01853d776ce4
Author: Pablo Mazzini <pmazzini@gmail.com>
Subject: rtc: sun6i: add Allwinner B288 compatible

This commit adds support for the Allwinner B288 RTC by reusing the A23
clock configuration. It maps the new compatible string
allwinner,sun8i-b288-rtc to the existing initialization functions.

> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 7fc60880e4094..05004e80ac089 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -350,6 +350,10 @@ static void __init sun8i_a23_rtc_clk_init(struct device_node *node)
>  CLK_OF_DECLARE_DRIVER(sun8i_a23_rtc_clk, "allwinner,sun8i-a23-rtc",
>  		      sun8i_a23_rtc_clk_init);
>  
> +/* The B288 RTC clocks are the same as the A23's */
> +CLK_OF_DECLARE_DRIVER(sun8i_b288_rtc_clk, "allwinner,sun8i-b288-rtc",
> +		      sun8i_a23_rtc_clk_init);
> +

[Severity: High]
Does adding this compatible string expose an issue in the error handling
of sun6i_rtc_clk_init on registration failure?

If clk_register fails for rtc->losc, the function returns an error
pointer and jumps to the err_register label. Since the global sun6i_rtc
pointer is already assigned to the allocated rtc struct earlier:

sun6i_rtc_clk_init()
    /* Yes, I know, this is ugly. */
    sun6i_rtc = rtc;
    ...
    rtc->losc = clk_register(NULL, &rtc->hw);
    if (IS_ERR(rtc->losc)) {
        pr_crit("Couldn't register the LOSC clock\n");
        goto err_register;
    }

The probe function sun6i_rtc_probe later fetches this dangling pointer:

sun6i_rtc_probe()
    struct sun6i_rtc_dev *chip = sun6i_rtc;
    ...
    clk_prepare_enable(chip->losc);

Can this lead to a kernel panic when clk_prepare_enable attempts to
dereference the error pointer stored in chip->losc?

[Severity: Medium]
Additionally, if clk_register succeeds for rtc->losc but a later clock
registration fails, triggering the jump to err_register, does this
cleanup path leak the rtc->losc clock?

sun6i_rtc_clk_init()
err_register:
    clk_hw_unregister_fixed_rate(rtc->int_osc);
err:
    kfree(clk_data);

The err_register path unregisters rtc->int_osc but appears to omit
calling clk_unregister(rtc->losc).

[Severity: Low]
Does the err cleanup path also leak the dynamically allocated rtc
structure?

sun6i_rtc_clk_init()
    rtc = kzalloc_obj(*rtc);
    ...
err:
    kfree(clk_data);

The error path frees clk_data but seems to omit freeing the allocated
rtc structure.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012100816.331033-1-pmazzini@gmail.com?part=6