Re: [PATCH -next v3 1/2] watchdog: Fix potential dereferencing of null pointer

[Guenter Roeck]

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@xxxxxxxxxx>
> >> ---
> >>   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
> >>
> > 
>