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

[wangwensheng (C)]

在 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.

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