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

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.
- [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.
- [Medium] The allocated and added platform device is permanently leaked if `platform_driver_register()` fails.
- [Medium] The watchdog driver silently ignores hardware access failures if the `bram_base` region cannot be claimed, falsely reporting success to the watchdog core.
--

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