Re: [PATCH 1/5] rtc: hym8563: Make sure clk_init_data is fully initialized

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

Pre-existing issues:
- [High] Use-after-free and resource leak on driver unbind or probe failure.
- [High] RTC alarm interrupts are acknowledged but not reported to the RTC subsystem, breaking alarm functionality.
--

--- Patch [1]: [PATCH 1/5] rtc: hym8563: Make sure clk_init_data is fully initialized ---
commit 706406627c1866b58ba6ad7ce0c506c6e63d88ad
Author: Geert Uytterhoeven <[email protected]>

rtc: hym8563: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock. This
commit ensures all members are fully initialized to avoid subtle issues
with uninitialized stack memory.

> diff --git a/drivers/rtc/rtc-hym8563.c b/drivers/rtc/rtc-hym8563.c
> index 3156aa5f2d9ff..1d52cc3def02e 100644
> --- a/drivers/rtc/rtc-hym8563.c
> +++ b/drivers/rtc/rtc-hym8563.c
> @@ -376,8 +376,8 @@ static struct clk *hym8563_clkout_register_clk(struct hym8563 *hym8563)
>  {
>  	struct i2c_client *client = hym8563->client;
>  	struct device_node *node = client->dev.of_node;
> +	struct clk_init_data init = {};
>  	struct clk *clk;
> -	struct clk_init_data init;
>  	int ret;
>  
>  	ret = i2c_smbus_write_byte_data(client, HYM8563_CLKOUT,

[Severity: High]
This isn't a bug introduced by this patch, but could there be a
use-after-free if devm_rtc_register_device() fails or if the driver is
unbound?

Looking at hym8563_clkout_register_clk(), it registers an unmanaged clock
and an OF clock provider using clk_register() and of_clk_add_provider().
However, the hym8563 structure containing the clkout_hw is allocated via
devm_kzalloc().

If devm_rtc_register_device() fails in hym8563_probe(), or if the driver
is later unbound (there is no .remove callback in hym8563_driver), the
devres framework will free the hym8563 structure. Because the clock is
never unregistered, the clock framework will retain a pointer to the
freed clkout_hw, leading to a crash if the clock is accessed later.

Should this use devm_clk_register() and devm_of_clk_add_hw_provider(), or
provide a .remove callback to clean up the clock and OF provider?

[Severity: High]
This is a pre-existing issue, but does the interrupt handler fail to
notify the RTC core of alarm events?

In hym8563_irq(), the handler reads and clears the hardware alarm flag
(HYM8563_CTL2_AF) to acknowledge the interrupt at the hardware level, but
it then exits like this:

out:
        rtc_unlock(hym8563->rtc);
        return IRQ_HANDLED;
}

It appears to never call rtc_update_irq(). Doesn't this mean rtc->irq_queue
is never woken up, causing any userspace process waiting for the alarm via
select(), poll(), or timerfd to stall indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1