Re: [PATCH 6/9] watchdog: w83627hf_wdt: Store Super I/O unlocking sequence in platform data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The local stack variable `struct resource res` is partially uninitialized before being passed to `platform_device_register_resndata()`, leaking stack garbage and risking kernel panics.
- [High] Missing NULL check for `pdata` (`pdev->dev.platform_data`) in `wdt_probe` leads to an unconditional NULL pointer dereference.
- [High] Missing NULL check for `id` (`platform_get_device_id(pdev)`) in `wdt_probe` leads to a kernel panic.
--

--- Patch [6]: [PATCH 6/9] watchdog: w83627hf_wdt: Store Super I/O unlocking sequence in platform data ---
commit a8ddfa0e2fa7007bc7d8b51099f9722c0113c5c5
Author: Paul Louvel <paul.louvel@bootlin.com>

watchdog: w83627hf_wdt: Store Super I/O unlocking sequence in platform data

This patch stores the Super I/O unlocking sequence in the platform data
instead of using global variables.

> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index 6363d571182fa..fed05fcf82bbf 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
[ ... ]
> @@ -441,13 +443,14 @@ static int wdt_find(int addr)
>  		pr_err("Unsupported chip ID: 0x%02x\n", val);
>  		break;
>  	}
> -	superio_exit(addr);
> +	superio_exit(addr, leave);
>  	return ret;
>  }
>  
>  static int wdt_probe(struct platform_device *pdev)
>  {
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	const struct wdt_pdata *pdata = pdev->dev.platform_data;
>  	enum chips chip = id->driver_data;

[Severity: High]
Is it possible for id to be NULL here? If this driver is bound to an
arbitrary device via the driver_override sysfs interface (which skips setting
pdev->id_entry), platform_get_device_id(pdev) will return NULL. Will this
cause a kernel panic when dereferencing id->driver_data?

>  	struct watchdog_device *wdd;
>  	struct w83627hf_data *data;
[ ... ]
> @@ -477,6 +480,8 @@ static int wdt_probe(struct platform_device *pdev)
>  	wdd->max_timeout = 255;
>  
>  	data->sioaddr = res->start;
> +	data->siocfg_enter = pdata->siocfg_enter;
> +	data->siocfg_leave = pdata->siocfg_leave;

[Severity: High]
Could pdata be NULL at this point? If a local privileged user manually
triggers driver binding via sysfs on a dummy platform device that lacks
platform_data, pdev->dev.platform_data will be NULL. Dereferencing pdata
here would result in a NULL pointer dereference.

>  	data->reg.timeout = W83627HF_WDT_TIMEOUT;
>  	data->reg.control = W83627HF_WDT_CONTROL;
>  	data->reg.csr = W836X7HF_WDT_CSR;
[ ... ]
> @@ -579,20 +589,22 @@ static struct platform_device *wdt_pdev;
>  
>  static int __init wdt_init(void)
>  {
> -	struct wdt_pdata pdata;
>  	struct resource res;

[Severity: High]
Does this code leave internal fields of the stack variable res uninitialized?
Since res is declared on the stack without being zero-initialized, fields
like parent, sibling, and child may contain stack garbage.

Later, when res is passed to platform_device_register_resndata(), it uses
kmemdup() to copy this struct. During platform_device_add(), the kernel
iterates over resources and checks if (!r->parent). Since res.parent
contains non-NULL stack garbage, it bypasses the default assignment and
passes the garbage pointer to insert_resource(), which could result in a
page fault and kernel panic during module load.

>  	int sioaddr;
>  	int ret;
>  	int chip;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-w83627hf_wdt-improvements-v1-0-4e9a1b4e8297@bootlin.com?part=6