Re: [PATCH] watchdog: pretimeout: Fix UAF in watchdog_unregister_governor()

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

Pre-existing issues:
- [High] A teardown symmetry violation in `watchdog_dev_unregister()` exposes a race window with sysfs, allowing a Use-After-Free of governor modules.
- [Medium] A sysfs race condition during device registration allows user-configured pretimeout governors to be silently overwritten by `default_gov`.
--

--- Patch [1]: [PATCH] watchdog: pretimeout: Fix UAF in watchdog_unregister_governor() ---
commit f31dd8ccfc4553641fb99ced744ddc61a7ad970c
Author: Tzung-Bi Shih <[email protected]>

watchdog: pretimeout: Fix UAF in watchdog_unregister_governor()

When a watchdog governor is unregistered, it updates existing watchdog
devices using it to fall back to default_gov. This commit clears
default_gov if it matches the governor being unregistered to prevent
two use-after-free scenarios where a dangling pointer is inherited.

> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> index 19eb2ed2c7cb0..02e09b9e396da 100644
> --- a/drivers/watchdog/watchdog_pretimeout.c
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -167,6 +167,8 @@ void watchdog_unregister_governor(struct watchdog_governor *gov)
>  	}
>  
>  	spin_lock_irq(&pretimeout_lock);
> +	if (default_gov == gov)
> +		default_gov = NULL;
>  	list_for_each_entry(p, &pretimeout_list, entry)

[Severity: High]
This is a pre-existing issue, but is there a teardown symmetry violation in
watchdog_dev_unregister() that exposes a race window with sysfs?

If watchdog_dev_unregister() calls watchdog_unregister_pretimeout() before
watchdog_cdev_unregister():

drivers/watchdog/watchdog_dev.c:watchdog_dev_unregister() {
	watchdog_unregister_pretimeout(wdd);
	watchdog_cdev_unregister(wdd);
}

The device is removed from pretimeout_list, but the sysfs interface is still
active. A concurrent write to the pretimeout_governor sysfs node can
successfully assign a governor to wdd->gov:

drivers/watchdog/watchdog_pretimeout.c:watchdog_pretimeout_governor_set() {
	spin_lock_irq(&pretimeout_lock);
	wdd->gov = priv->gov;
	spin_unlock_irq(&pretimeout_lock);
}

If the governor module is then unloaded, watchdog_unregister_governor() fails
to clear wdd->gov because the device is no longer in pretimeout_list. If an
in-flight hardware watchdog pretimeout interrupt fires,
watchdog_notify_pretimeout() dereferences the dangling wdd->gov pointer:

drivers/watchdog/watchdog_pretimeout.c:watchdog_notify_pretimeout() {
	wdd->gov->pretimeout(wdd);
}

Can this lead to a use-after-free?

>  		if (p->wdd->gov == gov)
>  			p->wdd->gov = default_gov;

[Severity: Medium]
This is also a pre-existing issue, but does watchdog_dev_register() silently
overwrite user-configured pretimeout governors due to a sysfs race condition?

When watchdog_dev_register() is called, it registers the character device and
sysfs interfaces before fully initializing the internal pretimeout state:

drivers/watchdog/watchdog_dev.c:watchdog_dev_register() {
	ret = watchdog_cdev_register(wdd);
	if (ret)
		return ret;

	ret = watchdog_register_pretimeout(wdd);
}

If a userspace udev rule or concurrent process immediately writes to the
pretimeout_governor sysfs node, setting wdd->gov, the subsequent call to
watchdog_register_pretimeout() unconditionally overwrites it with default_gov
without synchronizing with sysfs:

drivers/watchdog/watchdog_pretimeout.c:watchdog_register_pretimeout() {
	spin_lock_irq(&pretimeout_lock);
	list_add(&p->entry, &pretimeout_list);
	p->wdd = wdd;
	wdd->gov = default_gov;
	spin_unlock_irq(&pretimeout_lock);
}

Could this result in the user's requested governor being silently ignored?

>  	spin_unlock_irq(&pretimeout_lock);
>  
>  	mutex_unlock(&governor_lock);
>  }

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