Re: [PATCH] watchdog: lenovo_se30g2_se60_wdt: Watchdog for Lenovo SE30G2 and SE60

On Mon, Jun 22, 2026, at 10:56 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential 
> issue(s) to consider:
> - [High] The global `se_30g2_60_pdev` pointer is not cleared after 
> being freed on the `platform_device_add()` error path, leading to a 
> Use-After-Free and Double-Free during module exit.

I belive this is a valid issue - I'll fix in v2.

> - [High] SuperIO configuration mode is prematurely exited before 
> reading `bram_base`, causing the read to fail and the driver to map an 
> invalid I/O port address.

This one is I believe invalid and behaviour is correct. If someone wiser than me disagree's please let me know.

> - [Medium] The allocated and added platform device is permanently 
> leaked if `platform_driver_register()` fails.

I think this may be valid, I'll add a check in v2 if the device has been allocated and release if there is a driver registration failure.

> - [Medium] The watchdog driver silently ignores hardware access 
> failures if the `bram_base` region cannot be claimed, falsely reporting 
> success to the watchdog core.

I'll address this in v2

> --
>
> commit 9d10faee62683aacec801a77d1defa194acc32d0
> Author: Mark Pearson <[email protected]>
>
> watchdog: lenovo_se30g2_se60_wdt: Watchdog for Lenovo SE30G2 and SE60
>    
> Watchdog driver implementation for Lenovo SE30G2 and SE60 platforms.
>
>> diff --git a/drivers/watchdog/lenovo_se30g2_se60_wdt.c b/drivers/watchdog/lenovo_se30g2_se60_wdt.c
>> new file mode 100644
>> index 0000000000000..56e0e9ef8462b
>> --- /dev/null
>> +++ b/drivers/watchdog/lenovo_se30g2_se60_wdt.c
>> @@ -0,0 +1,274 @@
> [ ... ]
>> +static unsigned short lpc_chip_id(void)
>> +{
>> +	unsigned char msb, lsb;
>> +
>> +	outb(0x87, CFG_PORT);
>> +	outb(0x01, CFG_PORT);
>> +	outb(0x55, CFG_PORT);
>> +	outb(0x55, CFG_PORT);
>> +	msb = lpc_read(0x20);
>> +	lsb = lpc_read(0x21);
>> +	lpc_write(0x2, 0x2);
>
> [Severity: High]
> Does this lpc_write() prematurely exit SuperIO configuration mode?
>
> Later in se_30g2_60_wdt_probe(), the code attempts to select the BRAM
> logical device and read the base address:
>
> se_30g2_60_wdt_probe() {
>     ...
>     lpc_write(CFG_LDN, CFG_BRAM_LDN);
>     bram_base = (lpc_read(0x60) << 8) | lpc_read(0x61);
>     ...
> }
>
> If the configuration mode was already exited here, won't these subsequent
> reads return 0xFF, causing bram_base to be set to an invalid address?
>
>> +
>> +	return (msb << 8 | lsb);
>> +}
>
> [ ... ]
>
>> +static void wdt_write(unsigned short val)
>> +{
>> +	if (!request_muxed_region(bram_base, BRAM_SIZE, DRVNAME))
>> +		return;
>> +	bram_write(BRAM_WDT_REG, (val >> 8) & 0xFF);
>> +	bram_write(BRAM_WDT_REG + 1, val & 0xFF);
>> +	release_region(bram_base, BRAM_SIZE);
>> +}
>> +
>> +static int wdt_start(struct watchdog_device *wdog)
>> +{
>> +	wdt_write(wdog->timeout);
>> +	return 0;
>> +}
>
> [Severity: Medium]
> If request_muxed_region() fails in wdt_write(), the function silently returns.
>
> Since wdt_start() and wdt_ping() call wdt_write() and always return 0, won't
> this falsely report success to the watchdog core even if the hardware write
> failed?
>
> [ ... ]
>
>> +static int se_30g2_60_create_device(const struct dmi_system_id *id)
>> +{
>> +	int err;
>> +
>> +	se_30g2_60_pdev = platform_device_alloc("lenovo-se30g2-se60-wdt", -1);
>> +	if (!se_30g2_60_pdev)
>> +		return -ENOMEM;
>> +
>> +	err = platform_device_add(se_30g2_60_pdev);
>> +	if (err)
>> +		platform_device_put(se_30g2_60_pdev);
>
> [Severity: High]
> If platform_device_add() fails, platform_device_put() frees the device, but
> the global se_30g2_60_pdev pointer is not set to NULL.
>
> Since dmi_check_system() returns the number of matches regardless of callback
> errors, module initialization will proceed. When the module is unloaded,
> se_30g2_60_wdt_exit() will check if se_30g2_60_pdev is non-NULL and call
> platform_device_unregister(), leading to a use-after-free and double-free.
>
> Could se_30g2_60_pdev be explicitly set to NULL on the error path to avoid
> this?
>
>> +
>> +	return err;
>> +}
>
> [ ... ]
>
>> +static int __init se_30g2_60_wdt_init(void)
>> +{
>> +	if (!dmi_check_system(se_30g2_60))
>> +		return -ENODEV;
>> +
>> +	return platform_driver_register(&se_30g2_60_wdt_driver);
>> +}
>
> [Severity: Medium]
> If platform_driver_register() fails here, the platform device successfully
> allocated and added by the DMI callback is never unregistered.
>
> Will this permanently leak the platform device and cause subsequent module load
> attempts to fail with -EEXIST?
>
> -- 
> Sashiko AI review · 
> https://sashiko.dev/#/patchset/[email protected]?part=1

I'll hold off a couple of days on doing v2 in case there is any other feedback from maintainers outside of the sashiko recommendations.

Mark