Re: [PATCH v2 5/8] watchdog: w83627hf_wdt: Add Super I/O configuration port as a platform device resource

On 7/26/26 11:56, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Claiming the shared Super I/O configuration port as a static platform device resource causes a Use-After-Free and breaks global mutual exclusion upon module unload.
> --
> 
> --- Patch [5]: [PATCH v2 5/8] watchdog: w83627hf_wdt: Add Super I/O configuration port as a platform device resource ---
> commit 7147d011852b779a7e376729ecb29c06b86e9282
> Author: Paul Louvel <[email protected]>
> 
> watchdog: w83627hf_wdt: Add Super I/O configuration port as a platform device resource
> 
> This commit changes the Super I/O configuration port address from a
> global variable to a platform device resource. It uses
> platform_device_register_resndata() to assign the IO resource during
> module initialization.
> 
>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>> index 69d4230d8f6cc..a067b8ea6b289 100644
>> --- a/drivers/watchdog/w83627hf_wdt.c
>> +++ b/drivers/watchdog/w83627hf_wdt.c
> [ ... ]
>> @@ -594,8 +597,14 @@ static int __init wdt_init(void)
>>   	if (ret)
>>   		return ret;
>>   
>> -	wdt_pdev = platform_device_register_data(NULL, wdt_ids[chip].name,
>> -						 PLATFORM_DEVID_NONE, NULL, 0);
>> +	res.name = "Super I/O port";
>> +	res.flags = IORESOURCE_IO;
>> +	res.start = sioaddr;
>> +	res.end = sioaddr + 1;
>> +
>> +	wdt_pdev = platform_device_register_resndata(NULL, wdt_ids[chip].name,
>> +						     PLATFORM_DEVID_NONE, &res,
>> +						     1, NULL, 0);
> 
> [Severity: Critical]
> Does claiming the shared Super I/O configuration port as a static platform
> device resource cause a use-after-free regression and break mutual exclusion
> on module unload?
> 
> If this port is registered as a non-busy I/O resource, it becomes a
> container node in the global ioport_resource tree. When another driver
> calls request_muxed_region() for the same port, it might sleep on
> muxed_resource_wait while holding a parent pointer to this container node.
> 
> If the wdt module is unloaded while that thread is sleeping,
> platform_device_del() unlinks the container node and
> platform_device_release() frees its memory. When the sleeping thread
> wakes up, will it dereference the freed parent pointer in
> __request_resource()?
> 
> Additionally, it appears that unlinking the container node orphans any
> active child reservations from the tree. Could this allow concurrent
> hardware access to the shared Super I/O port?
> 

That makes me wonder: Under which authority can the watchdog driver claim
ownership of this resource ? What is the rationale ?

Why should the owner not be, for example, the hardware monitoring driver ?
Or the Bluetooth driver, if the chip has one ? Or some other driver ?
Or some non-existent Super-IO core ?

On a higher level, why even risk the problem outlined by Sashiko above ?
What is the benefit of doing this ? The patch description fails to
explain _why_ this change is made. Actually, that is true for many
of the patches. Quite often the description explain what is done,
but not the reason for it. The reason may be obvious for you, but not
or not always for me - and very much so for this patch.

Please explain why you are making those changes.

Also, I notice that at least in some cases it looks like you did not
address the feedback from Sashiko. Partially that is because problems
in one patch are addressed in a later patch of the series, but I am quite
sure I have seen Sashiko's feedback about the missing parent device
initialization before. Please ensure to address its feedback.

Thanks,
Guenter