Re: [PATCH v1] watchdog: unregister PM notifier on watchdog unregister

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Ignored errors during deferred watchdog registration and unhandled failure paths lead to Use-After-Free and NULL pointer dereferences upon unregistration.
- [High] Lockless access to `wd_data->wdd` in `watchdog_open()` leads to a Use-After-Free and violates the `watchdog_start()` locking contract.
--

commit 7739691e12f24218c1001ecfc408a279156feca5
Author: Yuho Choi <dbgh9129@gmail.com>

watchdog: unregister PM notifier on watchdog unregister

This commit ensures the PM notifier is unregistered before tearing down the
watchdog device. Previously, if WDOG_NO_PING_ON_SUSPEND was set, the embedded
notifier block would be left on the PM notifier chain after unregistration,
potentially leading to calls with a stale watchdog_device pointer.

> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 8300520688d07..6c087b4ef5dd5 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -391,6 +391,9 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
>  		unregister_reboot_notifier(&wdd->reboot_nb);
>  
> +	if (test_bit(WDOG_NO_PING_ON_SUSPEND, &wdd->status))
> +		unregister_pm_notifier(&wdd->pm_nb);
> +
>  	watchdog_dev_unregister(wdd);

[Severity: High]
This is a pre-existing issue, but I noticed that ignored errors during
deferred watchdog registration and unhandled failure paths can lead to a
Use-After-Free and NULL pointer dereferences upon unregistration.

When a watchdog driver registers early, watchdog_register_device() adds it to
wtd_deferred_reg_list and returns 0. Later, watchdog_deferred_registration()
processes the list:

drivers/watchdog/watchdog_core.c:watchdog_deferred_registration() {
    ...
		list_del(&wdd->deferred);
		__watchdog_register_device(wdd);
	}
	mutex_unlock(&wtd_deferred_reg_mutex);
}

The return value of __watchdog_register_device() is ignored here, concealing
fatal failures from drivers. If watchdog_cdev_register() fails, the error
paths free wd_data via put_device() but fail to clear wdd->wd_data:

drivers/watchdog/watchdog_dev.c:watchdog_cdev_register() {
    ...
	if (err) {
		pr_err("watchdog%d unable to add device %d:%d\n",
			wdd->id,  MAJOR(watchdog_devt), wdd->id);
		...
		put_device(&wd_data->dev);
		return err;
	}
    ...
}

When the driver is later unbound, it calls watchdog_unregister_device(), which
invokes watchdog_cdev_unregister(). This function unconditionally dereferences
wdd->wd_data:

drivers/watchdog/watchdog_dev.c:watchdog_cdev_unregister() {
    ...
	struct watchdog_core_data *wd_data = wdd->wd_data;

	cdev_device_del(&wd_data->cdev, &wd_data->dev);
    ...
}

Could this lead to a NULL pointer dereference or a Use-After-Free if deferred
registration fails, since wdd->wd_data was not cleared?

[Severity: High]
This is a pre-existing issue, but lockless access to wd_data->wdd in
watchdog_open() appears to lead to a Use-After-Free and violates the
watchdog_start() locking contract.

watchdog_open() fetches wdd = wd_data->wdd locklessly, failing to acquire
wd_data->lock to synchronize with concurrent teardown operations:

drivers/watchdog/watchdog_dev.c:watchdog_open() {
    ...
	wdd = wd_data->wdd;

	/*
	 * If the /dev/watchdog device is open, we don't want the module
	 * to be unloaded.
	 */
	hw_running = watchdog_hw_running(wdd);
	if (!hw_running && !try_module_get(wdd->ops->owner)) {
		err = -EBUSY;
		goto out_clear;
	}

	err = watchdog_start(wdd);
    ...
}

If a device unbind or hot-unplug occurs concurrently,
watchdog_cdev_unregister() acquires wd_data->lock, sets wd_data->wdd = NULL,
and unlocks:

drivers/watchdog/watchdog_dev.c:watchdog_cdev_unregister() {
    ...
	mutex_lock(&wd_data->lock);
	wd_data->wdd = NULL;
	wdd->wd_data = NULL;
	mutex_unlock(&wd_data->lock);
    ...
}

Does this allow watchdog_open() to dereference a freed wdd pointer if the
hardware driver finishes its remove() callback and frees wdd? Additionally,
is the locking contract for watchdog_start() violated since wd_data->lock
is not held?

>  	ida_free(&watchdog_ida, wdd->id);
>  }

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