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

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 uninit=
ialized before being passed to `platform_device_register_resndata()`, leaki=
ng 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 `w=
dt_probe` leads to a kernel panic.
--

--- Patch [6]: [PATCH 6/9] watchdog: w83627hf_wdt: Store Super I/O unlockin=
g sequence in platform data ---
commit a8ddfa0e2fa7007bc7d8b51099f9722c0113c5c5
Author: Paul Louvel <[email protected]>

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;
>  }
> =20
>  static int wdt_probe(struct platform_device *pdev)
>  {
>  	const struct platform_device_id *id =3D platform_get_device_id(pdev);
> +	const struct wdt_pdata *pdata =3D pdev->dev.platform_data;
>  	enum chips chip =3D 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 setti=
ng
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 =3D 255;
> =20
>  	data->sioaddr =3D res->start;
> +	data->siocfg_enter =3D pdata->siocfg_enter;
> +	data->siocfg_leave =3D 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 =3D W83627HF_WDT_TIMEOUT;
>  	data->reg.control =3D W83627HF_WDT_CONTROL;
>  	data->reg.csr =3D W836X7HF_WDT_CSR;
[ ... ]
> @@ -579,20 +589,22 @@ static struct platform_device *wdt_pdev;
> =20
>  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 uninitialize=
d?
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;

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260725-w83627hf_w=
[email protected]?part=3D6