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

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 <[email protected]>

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/[email protected]?part=1