| Message ID | 20201102074413.69552-1-wangwensheng4@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] watchdog: Fix potential dereferencing of null pointer | expand |
On 11/1/20 11:44 PM, Wang Wensheng wrote: > A reboot notifier, which stops the WDT by calling the stop hook without > any check, would be registered when we set WDOG_STOP_ON_REBOOT flag. > > Howerer we allow the WDT driver to omit the stop hook since commit > "d0684c8a93549" ("watchdog: Make stop function optional") and provide > a module parameter for user that controls the WDOG_STOP_ON_REBOOT flag > in commit 9232c80659e94 ("watchdog: Add stop_on_reboot parameter to > control reboot policy"). Together that commits make user potential to > insert a watchdog driver that don't provide a stop hook but with the > stop_on_reboot parameter set, then dereferencing of null pointer occurs > on system reboot. > > Check the stop hook before registering the reboot notifier to fix the > issue. > This does a bit more: It also cleans up error handling. Which makes sense, but should be done in a second patch. Guenter > Fixes: d0684c8a9354 ("watchdog: Make stop function optional") > Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com> > --- > drivers/watchdog/watchdog_core.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index 423844757812..5269761ba072 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -252,10 +252,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > wdd->id = id; > > ret = watchdog_dev_register(wdd); > - if (ret) { > - ida_simple_remove(&watchdog_ida, id); > - return ret; > - } > + if (ret) > + goto id_remove; > } > > /* Module parameter to force watchdog policy on reboot. */ > @@ -267,15 +265,17 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > } > > if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { > - wdd->reboot_nb.notifier_call = watchdog_reboot_notifier; > + if (!wdd->ops->stop) { > + ret = -EINVAL; > + goto dev_unregister; > + } > > + wdd->reboot_nb.notifier_call = watchdog_reboot_notifier; > ret = register_reboot_notifier(&wdd->reboot_nb); > if (ret) { > pr_err("watchdog%d: Cannot register reboot notifier (%d)\n", > wdd->id, ret); > - watchdog_dev_unregister(wdd); > - ida_simple_remove(&watchdog_ida, id); > - return ret; > + goto dev_unregister; > } > } > > @@ -289,6 +289,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > } > > return 0; > + > +dev_unregister: > + watchdog_dev_unregister(wdd); > +id_remove: > + ida_simple_remove(&watchdog_ida, id); > + > + return ret; > } > > /** >
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 423844757812..5269761ba072 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -252,10 +252,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd) wdd->id = id; ret = watchdog_dev_register(wdd); - if (ret) { - ida_simple_remove(&watchdog_ida, id); - return ret; - } + if (ret) + goto id_remove; } /* Module parameter to force watchdog policy on reboot. */ @@ -267,15 +265,17 @@ static int __watchdog_register_device(struct watchdog_device *wdd) } if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { - wdd->reboot_nb.notifier_call = watchdog_reboot_notifier; + if (!wdd->ops->stop) { + ret = -EINVAL; + goto dev_unregister; + } + wdd->reboot_nb.notifier_call = watchdog_reboot_notifier; ret = register_reboot_notifier(&wdd->reboot_nb); if (ret) { pr_err("watchdog%d: Cannot register reboot notifier (%d)\n", wdd->id, ret); - watchdog_dev_unregister(wdd); - ida_simple_remove(&watchdog_ida, id); - return ret; + goto dev_unregister; } } @@ -289,6 +289,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd) } return 0; + +dev_unregister: + watchdog_dev_unregister(wdd); +id_remove: + ida_simple_remove(&watchdog_ida, id); + + return ret; } /**
A reboot notifier, which stops the WDT by calling the stop hook without any check, would be registered when we set WDOG_STOP_ON_REBOOT flag. Howerer we allow the WDT driver to omit the stop hook since commit "d0684c8a93549" ("watchdog: Make stop function optional") and provide a module parameter for user that controls the WDOG_STOP_ON_REBOOT flag in commit 9232c80659e94 ("watchdog: Add stop_on_reboot parameter to control reboot policy"). Together that commits make user potential to insert a watchdog driver that don't provide a stop hook but with the stop_on_reboot parameter set, then dereferencing of null pointer occurs on system reboot. Check the stop hook before registering the reboot notifier to fix the issue. Fixes: d0684c8a9354 ("watchdog: Make stop function optional") Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com> --- drivers/watchdog/watchdog_core.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)