Re: [PATCH 2/5] rtc: m41t80: Make sure clk_init_data is fully initialized

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup of clock registration and OF provider on probe failure and device unbind leads to memory leaks and Use-After-Free.
- [High] Missing cleanup of watchdog device and reboot notifier on probe failure causes resource leaks and Use-After-Free.
- [High] The seconds register is corrupted with the flags register value during time setting.
- [High] Clock operations race with RTC operations causing I2C register corruption.
--

--- Patch [2]: [PATCH 2/5] rtc: m41t80: Make sure clk_init_data is fully initialized ---
commit 11533c947dd31e7928f8df0ec766e6dae1e6ced8
Author: Geert Uytterhoeven <geert+renesas@glider.be>

rtc: m41t80: 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, prompting
drivers to initialize only the members they need. Make sure all members are
fully initialized, to avoid bugs and prevent future breakage.

> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 3c8c379392c14..ec86ca3f2c4c8 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -574,8 +574,8 @@ static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
>  	struct i2c_client *client = m41t80->client;
>  	struct device_node *node = client->dev.of_node;
>  	struct device_node *fixed_clock;
> +	struct clk_init_data init = {};
>  	struct clk *clk;
> -	struct clk_init_data init;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does m41t80_sqw_register_clk() leak the
registered clock and OF provider?

If devm_rtc_register_device() fails in m41t80_probe(), or when the driver
unbinds and calls m41t80_remove(), the m41t80_data struct is freed. However,
the clock and its OF provider remain registered:

m41t80_sqw_register_clk() {
    ...
    clk = clk_register(&client->dev, &m41t80->sqw);
    if (!IS_ERR(clk))
        of_clk_add_provider(node, of_clk_src_simple_get, clk);
    ...
}

Will subsequent clock operations dereference the freed m41t80->sqw field?

[Severity: High]
This is a pre-existing issue, but is there a missing cleanup of the watchdog
device and reboot notifier on probe failure?

In m41t80_probe(), the watchdog device and reboot notifier are registered:

m41t80_probe() {
    ...
    rc = misc_register(&wdt_dev);
    if (rc)
        return rc;
    rc = register_reboot_notifier(&wdt_notifier);
    ...
}

If devm_rtc_register_device() subsequently fails, the driver returns the
error directly without deregistering the watchdog device or the reboot
notifier:

m41t80_probe() {
    ...
    rc = devm_rtc_register_device(m41t80_data->rtc);
    if (rc)
        return rc;
    ...
}

Could this lead to a use-after-free if userspace then accesses /dev/watchdog,
causing wdt_ping() to dereference the freed clientdata memory?

[Severity: High]
This is a pre-existing issue, but does m41t80_rtc_set_time() inadvertently
corrupt the seconds register?

During time setting, when attempting to clear the STOP bit in the seconds
register, it writes the flags variable instead:

m41t80_rtc_set_time() {
    ...
    err = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
                                    flags & ~M41T80_SEC_ST);
    ...
}

Since flags is populated earlier by reading the FLAGS register
(M41T80_REG_FLAGS), would this write the raw value of the FLAGS register into
the seconds register, overwriting the previously set time?

[Severity: High]
This is a pre-existing issue, but can clock operations race with RTC
operations and corrupt I2C registers?

The CCF clock callbacks perform read-modify-write sequences on the
M41T80_REG_ALARM_MON and M41T80_REG_WDAY registers without holding a lock
that protects against concurrent time-setting operations:

m41t80_sqw_control() {
    ...
    int ret = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_MON);
    ...
    ret = i2c_smbus_write_byte_data(client, M41T80_REG_ALARM_MON, ret);
    ...
}

m41t80_sqw_set_rate() {
    ...
    reg = i2c_smbus_read_byte_data(client, reg_sqw);
    ...
    ret = i2c_smbus_write_byte_data(client, reg_sqw, reg);
    ...
}

Meanwhile, the RTC core callbacks modify these same registers under the
protection of the RTC subsystem's rtc_lock.

Since the CCF operations execute asynchronously under clock framework locks
and do not acquire the rtc_lock, could concurrent modifications by both
subsystems clobber the shared I2C registers?

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