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

["Paul Louvel" <paul.louvel@bootlin.com>]

Hi Guenter,

Sorry for not responding to your email earlier.

On Mon Jul 27, 2026 at 3:08 AM CEST, Guenter Roeck wrote:
> On 7/26/26 11:56, sashiko-bot@kernel.org 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 <paul.louvel@bootlin.com>
>> 
>> 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 ?

Yes, it should not.
Was a misunderstanding of my part. Did not know that adding a resource in driver
registration would cause resource contention if other drivers try to access the
SIO extended function mode.

>
> 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.

My bad if it was not clear.
This series is an attempt of modernizing this driver to a platform driver so
that it can use modern kernel APIs, expose sysfs interface, and clean the
driver.

And also add minute mode counting that is a feature that is just missing from
the driver. Maybe it should be a subject for another patch, outside of this
series ?

>
> 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.

Sashiko reviews will be taken into account for the next iteration.

>
> Thanks,
> Guenter

Thanks,
Paul.




-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com