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

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

[Myeonghun Pak]

The watchdog miscdevice and reboot notifier are registered before the RTC
device. If RTC device registration fails, probe returns without undoing
either registration. The remove callback is not called after probe
failure, leaving callbacks tied to a device whose driver failed to bind.
Those callbacks continue to dereference the file-global save_client.

Unregister the notifier and miscdevice before returning the RTC
registration error.

Fixes: 10d0c768cc6d ("rtc: m41t80: fix race conditions")
Cc: stable@vger.kernel.org
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/rtc/rtc-m41t80.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
--- 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) {
+#ifdef CONFIG_RTC_DRV_M41T80_WDT
+		if (m41t80_data->features & M41T80_FEATURE_HT) {
+			unregister_reboot_notifier(&wdt_notifier);
+			misc_deregister(&wdt_dev);
+		}
+#endif
 		return rc;
+	}
 
 	return 0;
 }
-- 
2.50.1

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

[sashiko-bot]

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