| Message ID | 20201105123848.93735-1-wangwensheng4@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [-next,v3,1/2] watchdog: Fix potential dereferencing of null pointer | expand |
On Thu, Nov 05, 2020 at 12:38:47PM +0000, 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. > > Fixes: d0684c8a9354 ("watchdog: Make stop function optional") > Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com> > --- > drivers/watchdog/watchdog_core.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index 423844757812..945ab38b14b8 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -267,8 +267,15 @@ 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) { > + pr_err("watchdog%d: Cannot support stop_on_reboot\n", > + wdd->id); > + watchdog_dev_unregister(wdd); > + ida_simple_remove(&watchdog_ida, id); > + return -EINVAL; > + } The problem with this is that setting the "stop_on_reboot" module parameter would now prevent the watchdog from being loaded, which isn't really desirable and might go unnoticed. I think the initial check should be above, with the "Mandatory operations" check, and if (stop_on_reboot != -1) { should be extended to if (stop_on_reboot != -1 && wdd->ops->stop) { or possibly more fancy: if (stop_on_reboot != -1) { if (stop_on_reboot) { if (!wdd->ops->stop) pr_warn("watchdog%d: stop_on_reboot not supported\n", wdd->id); else set_bit(WDOG_STOP_ON_REBOOT, &wdd->status); } else { clear_bit(WDOG_STOP_ON_REBOOT, &wdd->status); } } Thanks, Guenter > > + 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", > -- > 2.25.0 >
在 2020/11/5 22:26, Guenter Roeck 写道: > On Thu, Nov 05, 2020 at 12:38:47PM +0000, 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. >> >> Fixes: d0684c8a9354 ("watchdog: Make stop function optional") >> Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com> >> --- >> drivers/watchdog/watchdog_core.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c >> index 423844757812..945ab38b14b8 100644 >> --- a/drivers/watchdog/watchdog_core.c >> +++ b/drivers/watchdog/watchdog_core.c >> @@ -267,8 +267,15 @@ 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) { >> + pr_err("watchdog%d: Cannot support stop_on_reboot\n", >> + wdd->id); >> + watchdog_dev_unregister(wdd); >> + ida_simple_remove(&watchdog_ida, id); >> + return -EINVAL; >> + } > > The problem with this is that setting the "stop_on_reboot" module parameter > would now prevent the watchdog from being loaded, which isn't really > desirable and might go unnoticed. I think the initial check should be > above, with the "Mandatory operations" check, and > if (stop_on_reboot != -1) { > should be extended to > if (stop_on_reboot != -1 && wdd->ops->stop) { > > or possibly more fancy: > > if (stop_on_reboot != -1) { > if (stop_on_reboot) { > if (!wdd->ops->stop) > pr_warn("watchdog%d: stop_on_reboot not supported\n", wdd->id); > else > set_bit(WDOG_STOP_ON_REBOOT, &wdd->status); > } else { > clear_bit(WDOG_STOP_ON_REBOOT, &wdd->status); > } > } > > Thanks, > Guenter Now the divergence is that should we stop the registering process and return error when the STOP_ON_REBOOT flag is set but the driver doesn't support it. The flag is set in two scenes. Firstly,the driver that should provide the stop hook may set the flag staticlly, and it is a bug of the driver if it set the flag but without a stop hook. Then giving an error shall be more striking. Secondly, the user can change the flag using module parameter. Is it reasonable to just ignore the STOP_ON_REBOOT flag and give a warning when the user truely want it? And under this circumstance a warning is easier to get unnoticed than an error. I prefer to stop the registering process and return an error in those two scenes. Thanks > >> >> + 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", >> -- >> 2.25.0 >> >
On Fri, Nov 06, 2020 at 07:37:08AM +0000, wangwensheng (C) wrote: > 在 2020/11/5 22:26, Guenter Roeck 写道: > > On Thu, Nov 05, 2020 at 12:38:47PM +0000, 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. > >> > >> Fixes: d0684c8a9354 ("watchdog: Make stop function optional") > >> Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com> > >> --- > >> drivers/watchdog/watchdog_core.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > >> index 423844757812..945ab38b14b8 100644 > >> --- a/drivers/watchdog/watchdog_core.c > >> +++ b/drivers/watchdog/watchdog_core.c > >> @@ -267,8 +267,15 @@ 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) { > >> + pr_err("watchdog%d: Cannot support stop_on_reboot\n", > >> + wdd->id); > >> + watchdog_dev_unregister(wdd); > >> + ida_simple_remove(&watchdog_ida, id); > >> + return -EINVAL; > >> + } > > > > The problem with this is that setting the "stop_on_reboot" module parameter > > would now prevent the watchdog from being loaded, which isn't really > > desirable and might go unnoticed. I think the initial check should be > > above, with the "Mandatory operations" check, and > > if (stop_on_reboot != -1) { > > should be extended to > > if (stop_on_reboot != -1 && wdd->ops->stop) { > > > > or possibly more fancy: > > > > if (stop_on_reboot != -1) { > > if (stop_on_reboot) { > > if (!wdd->ops->stop) > > pr_warn("watchdog%d: stop_on_reboot not supported\n", wdd->id); > > else > > set_bit(WDOG_STOP_ON_REBOOT, &wdd->status); > > } else { > > clear_bit(WDOG_STOP_ON_REBOOT, &wdd->status); > > } > > } > > > > Thanks, > > Guenter > > Now the divergence is that should we stop the registering process and > return error when the STOP_ON_REBOOT flag is set but the driver doesn't > support it. The flag is set in two scenes. > Firstly,the driver that should provide the stop hook may set the flag > staticlly, and it is a bug of the driver if it set the flag but without > a stop hook. Then giving an error shall be more striking. > Secondly, the user can change the flag using module parameter. Is it > reasonable to just ignore the STOP_ON_REBOOT flag and give a warning > when the user truely want it? And under this circumstance a warning is > easier to get unnoticed than an error. > I prefer to stop the registering process and return an error in those > two scenes. > I disagree. A bad module parameter should not result in module load failures. Guenter > Thanks > > > >> > >> + 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", > >> -- > >> 2.25.0 > >> > > >
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 423844757812..945ab38b14b8 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -267,8 +267,15 @@ 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) { + pr_err("watchdog%d: Cannot support stop_on_reboot\n", + wdd->id); + watchdog_dev_unregister(wdd); + ida_simple_remove(&watchdog_ida, id); + return -EINVAL; + } + 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",
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 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)