2023-04-18 17:30:17

by Johannes Thumshirn

Subject: [PATCH 1/2] watchdog: menz069_wdt: fix watchdog initialisation

Doing a 'cat /dev/watchdog0' with menz069_wdt as watchdog0 will result in
a NULL pointer dereference.

This happens because we're passing the wrong pointer to
watchdog_register_device(). Fix this by getting rid of the static
watchdog_device structure and use the one embedded into the driver's
per-instance private data.

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/watchdog/menz69_wdt.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
index 8973f98bc6a5..bca0938f3429 100644
--- a/drivers/watchdog/menz69_wdt.c
+++ b/drivers/watchdog/menz69_wdt.c
@@ -98,14 +98,6 @@ static const struct watchdog_ops men_z069_ops = {
.set_timeout = men_z069_wdt_set_timeout,
};

-static struct watchdog_device men_z069_wdt = {
- .info = &men_z069_info,
- .ops = &men_z069_ops,
- .timeout = MEN_Z069_DEFAULT_TIMEOUT,
- .min_timeout = 1,
- .max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ,
-};
-
static int men_z069_probe(struct mcb_device *dev,
const struct mcb_device_id *id)
{
@@ -125,15 +117,19 @@ static int men_z069_probe(struct mcb_device *dev,
goto release_mem;

drv->mem = mem;
+ drv->wdt.info = &men_z069_info;
+ drv->wdt.ops = &men_z069_ops;
+ drv->wdt.timeout = MEN_Z069_DEFAULT_TIMEOUT;
+ drv->wdt.min_timeout = 1;
+ drv->wdt.max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ;

- drv->wdt = men_z069_wdt;
watchdog_init_timeout(&drv->wdt, 0, &dev->dev);
watchdog_set_nowayout(&drv->wdt, nowayout);
watchdog_set_drvdata(&drv->wdt, drv);
drv->wdt.parent = &dev->dev;
mcb_set_drvdata(dev, drv);

- return watchdog_register_device(&men_z069_wdt);
+ return watchdog_register_device(&drv->wdt);

release_mem:
mcb_release_mem(mem);
--
2.39.2

2023-04-18 18:21:51

by Guenter Roeck

Subject: Re: [PATCH 1/2] watchdog: menz069_wdt: fix watchdog initialisation

On 4/18/23 10:25, Johannes Thumshirn wrote:
> Doing a 'cat /dev/watchdog0' with menz069_wdt as watchdog0 will result in
> a NULL pointer dereference.
>
> This happens because we're passing the wrong pointer to
> watchdog_register_device(). Fix this by getting rid of the static
> watchdog_device structure and use the one embedded into the driver's
> per-instance private data.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/watchdog/menz69_wdt.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
> index 8973f98bc6a5..bca0938f3429 100644
> --- a/drivers/watchdog/menz69_wdt.c
> +++ b/drivers/watchdog/menz69_wdt.c
> @@ -98,14 +98,6 @@ static const struct watchdog_ops men_z069_ops = {
> .set_timeout = men_z069_wdt_set_timeout,
> };
>
> -static struct watchdog_device men_z069_wdt = {
> - .info = &men_z069_info,
> - .ops = &men_z069_ops,
> - .timeout = MEN_Z069_DEFAULT_TIMEOUT,
> - .min_timeout = 1,
> - .max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ,
> -};
> -
> static int men_z069_probe(struct mcb_device *dev,
> const struct mcb_device_id *id)
> {
> @@ -125,15 +117,19 @@ static int men_z069_probe(struct mcb_device *dev,
> goto release_mem;
>
> drv->mem = mem;
> + drv->wdt.info = &men_z069_info;
> + drv->wdt.ops = &men_z069_ops;
> + drv->wdt.timeout = MEN_Z069_DEFAULT_TIMEOUT;
> + drv->wdt.min_timeout = 1;
> + drv->wdt.max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ;
>
> - drv->wdt = men_z069_wdt;
> watchdog_init_timeout(&drv->wdt, 0, &dev->dev);
> watchdog_set_nowayout(&drv->wdt, nowayout);
> watchdog_set_drvdata(&drv->wdt, drv);
> drv->wdt.parent = &dev->dev;
> mcb_set_drvdata(dev, drv);
>
> - return watchdog_register_device(&men_z069_wdt);
> + return watchdog_register_device(&drv->wdt);
>
> release_mem:
> mcb_release_mem(mem);