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

Message ID 20201105123848.93735-1-wangwensheng4@huawei.com
State New
Headers show
Series
  • [-next,v3,1/2] watchdog: Fix potential dereferencing of null pointer
Related show

Commit Message

Wang Wensheng Nov. 5, 2020, 12:38 p.m.
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(-)

Comments

Guenter Roeck Nov. 5, 2020, 2:26 p.m. | #1
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

>
Wang Wensheng Nov. 6, 2020, 7:37 a.m. | #2
在 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

>>

>
Guenter Roeck Nov. 6, 2020, 1:19 p.m. | #3
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

> >>

> > 

>

Patch

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