Re: [PATCH] rtc: m41t80: clean up watchdog on registration failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The clock provider registered by `m41t80_sqw_register_clk()` is never unregistered on probe failure or device removal, leaving a dangling pointer in the Common Clock Framework.
- [High] Probing multiple M41T80 devices overwrites the global `save_client` pointer before verifying watchdog registration success, leading to a NULL pointer dereference.
- [High] Registering the watchdog misc device before probe completes allows user-space to open the device, and the lack of VFS file descriptor revocation coupled with a dangling `save_client` leads to a NULL pointer dereference.
--

--- Patch [1]: [PATCH] rtc: m41t80: clean up watchdog on registration failure ---
commit 7f474c7e546e109c3aad769a8ec54e1bdf1316d0
Author: Myeonghun Pak <mhun512@gmail.com>

rtc: m41t80: clean up watchdog on registration failure

This commit unregisters the watchdog miscdevice and reboot notifier if the
RTC device registration fails during probe. This prevents leaving callbacks
tied to a device whose driver failed to bind.

> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 3c8c379392c14..3cb3f5e94f861 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -1008,8 +1008,15 @@ static int m41t80_probe(struct i2c_client *client)
>  #endif
>  
>  	rc = devm_rtc_register_device(m41t80_data->rtc);
> -	if (rc)
> +	if (rc) {

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path also
need to unregister the clock provider?

If devm_rtc_register_device() fails, the driver frees the devres-managed
m41t80_data. However, m41t80_sqw_register_clk() registers the clock using
non-devm functions (clk_register and of_clk_add_provider):

drivers/rtc/rtc-m41t80.c: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);
    ...
}

Since neither this error path nor m41t80_remove() unregisters the clock,
could subsequent accesses to the clock tree (like reading
/sys/kernel/debug/clk/clk_summary) dereference the freed clk_hw and
cause a use-after-free?

[Severity: High]
This is another pre-existing issue, but is it safe to unconditionally
overwrite the global save_client pointer earlier in the probe function?

Earlier in m41t80_probe(), save_client is set before misc_register() is
confirmed to succeed:

drivers/rtc/rtc-m41t80.c:m41t80_probe() {
    ...
    save_client = client;
    rc = misc_register(&wdt_dev);
    if (rc)
        return rc;
    ...
}

If multiple M41T80 devices exist, the second probe will overwrite save_client
but fail at misc_register() because the watchdog minor is already taken.
The driver core then clears drvdata for the failed device.

If a user-space process sends a keepalive to the working watchdog, wdt_ping()
will use the corrupted save_client pointing to the unbound device, fetching
a NULL clientdata. Could this cause a NULL pointer dereference in wdt_ping()?

> +#ifdef CONFIG_RTC_DRV_M41T80_WDT
> +		if (m41t80_data->features & M41T80_FEATURE_HT) {
> +			unregister_reboot_notifier(&wdt_notifier);
> +			misc_deregister(&wdt_dev);

[Severity: High]
This is a pre-existing issue, but should save_client be cleared to NULL
when deregistering the watchdog?

When misc_deregister() is called here (and in m41t80_remove()), it removes
the device node but does not revoke existing open file descriptors. If
user-space has already opened /dev/watchdog before the probe fails, the
global save_client pointer is left dangling.

If user-space later writes to the open file descriptor, wdt_ping() will use
save_client to fetch clientdata:

drivers/rtc/rtc-m41t80.c:wdt_ping() {
    ...
    struct m41t80_data *clientdata = i2c_get_clientdata(save_client);
    ...
    if (clientdata->features & M41T80_FEATURE_WD)
    ...
}

Since the driver core clears drvdata on probe failure, clientdata will be
NULL. Does this lead to a NULL pointer dereference?

> +		}
> +#endif
>  		return rc;
> +	}
>  
>  	return 0;
>  }

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