[PATCH v2] watchdog: take all OF aliases into account when assigning id

[PATCH v2] watchdog: take all OF aliases into account when assigning id

[Rasmus Villemoes]

If some, but not all, watchdog devices have device tree aliases, those
without aliases might (depending on probe order) be assigned an id
which would otherwise be assigned to one of those with an alias.

This is problematic when for example watchdog0 is an alias for an
always-running gpio watchdog that userspace must handle, but the SOC's
watchdog device(s) get probed first and thus one of those become
/dev/watchdog0, and then at some point later, the gpio watchdog device
gets probed and becomes /dev/watchdog5.

Ensure that ids for devices without a device tree alias are allocated
from among those where no watchdogX alias exists.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

v2: Allocate dynamic ids from among all non-alias ids, not merely from
above the highest existing alias.  This ensures that if there is no
watchdog0 alias, the first non-aliased watchdog device to get probed
will get id 0, and thus become /dev/watchdog0 and /dev/watchdog.

 drivers/watchdog/watchdog_core.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index f4097aefaf49..2293e885cd92 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -240,7 +240,9 @@ EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
 
 static int ___watchdog_register_device(struct watchdog_device *wdd)
 {
-	int ret, id = -1;
+	int ret, min_id, id = -1;
+	struct device_node *np;
+	char alias[16];
 
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -EINVAL;
@@ -265,8 +267,26 @@ static int ___watchdog_register_device(struct watchdog_device *wdd)
 					     GFP_KERNEL);
 	}
 
-	if (id < 0)
-		id = ida_alloc_max(&watchdog_ida, MAX_DOGS - 1, GFP_KERNEL);
+	/*
+	 * Find an id which is not pre-assigned via a DT alias to some
+	 * other, possibly not yet probed, watchdog device.
+	 */
+	if (id < 0) {
+		np = of_find_node_by_path("/aliases");
+
+		for (min_id = 0; ; min_id = id + 1) {
+			id = ida_alloc_range(&watchdog_ida, min_id, MAX_DOGS - 1,
+					     GFP_KERNEL);
+			if (!np || id < 0)
+				break;
+
+			snprintf(alias, sizeof(alias), "watchdog%d", id);
+			if (!of_get_property(np, alias, NULL))
+				break;
+			ida_free(&watchdog_ida, id);
+		}
+		of_node_put(np);
+	}
 
 	if (id < 0)
 		return id;
-- 
2.55.0

Re: [PATCH v2] watchdog: take all OF aliases into account when assigning id

[sashiko-bot]

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

New issues:
- [Medium] The fallback ID allocation for legacy watchdog modules bypasses the new alias-aware logic, potentially stealing an ID reserved by a DT alias.

Pre-existing issues:
- [High] A pre-existing flaw in deferred watchdog registration ignores failures, leaving dangling pointers that trigger Use-After-Free and Double Free vulnerabilities when the device is subsequently unbound.
--

--- Patch [1]: [PATCH v2] watchdog: take all OF aliases into account when assigning id ---
commit 723b5098283b03dc095379b11c599fe692046472
Author: Rasmus Villemoes <linux@rasmusvillemoes.dk>

watchdog: take all OF aliases into account when assigning id

Ensure that ids for devices without a device tree alias are allocated
from among those where no watchdogX alias exists.

> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c

[ ... ]

> @@ -265,8 +267,26 @@ static int ___watchdog_register_device(struct watchdog_device *wdd)
>  					     GFP_KERNEL);
>  	}
>  
> -	if (id < 0)
> -		id = ida_alloc_max(&watchdog_ida, MAX_DOGS - 1, GFP_KERNEL);
> +	/*
> +	 * Find an id which is not pre-assigned via a DT alias to some
> +	 * other, possibly not yet probed, watchdog device.
> +	 */
> +	if (id < 0) {
> +		np = of_find_node_by_path("/aliases");
> +
> +		for (min_id = 0; ; min_id = id + 1) {
> +			id = ida_alloc_range(&watchdog_ida, min_id, MAX_DOGS - 1,
> +					     GFP_KERNEL);
> +			if (!np || id < 0)
> +				break;
> +
> +			snprintf(alias, sizeof(alias), "watchdog%d", id);
> +			if (!of_get_property(np, alias, NULL))
> +				break;
> +			ida_free(&watchdog_ida, id);
> +		}
> +		of_node_put(np);
> +	}
>  
>  	if (id < 0)
>  		return id;
>  	wdd->id = id;
>  
>  	ret = watchdog_dev_register(wdd);
>  	if (ret) {
>  		ida_free(&watchdog_ida, id);
>  		if (!(id == 0 && ret == -EBUSY))
>  			return ret;
>  
>  		/* Retry in case a legacy watchdog module exists */
>  		id = ida_alloc_range(&watchdog_ida, 1, MAX_DOGS - 1,
>  				     GFP_KERNEL);

[Severity: Medium]
Does this fallback allocation for legacy modules need to validate against
DT aliases as well?

Because this path calls ida_alloc_range() directly, it bypasses the newly
introduced alias validation loop. If a legacy watchdog module causes a
retry, could this allocation steal an ID that is actively reserved for
another unprobed device via a DT alias?

[Severity: High]
This is a pre-existing issue, but I noticed a potential flaw in how
watchdog_deferred_registration() handles failures. 

If the registration fails, the return value is ignored:

drivers/watchdog/watchdog_core.c:watchdog_deferred_registration() {
    ...
    __watchdog_register_device(wdd);
    ...
}

During the failed registration attempt, watchdog_cdev_register() assigns
wdd->wd_data, but never clears it on the error path:

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

If this fails, the device is left in a partially initialized state but
remains registered in devres. When the device is subsequently unbound or
the module removed, __watchdog_unregister_device() executes on the failed
device:

drivers/watchdog/watchdog_core.c:__watchdog_unregister_device() {
    ...
    watchdog_dev_unregister(wdd);
    ida_free(&watchdog_ida, wdd->id);
    ...
}

Could this trigger a use-after-free or NULL pointer dereference inside
watchdog_dev_unregister(), as well as a double-free on the IDA since the
ID was already freed during the initial registration failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714105356.2931450-1-linux@rasmusvillemoes.dk?part=1