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

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

[Wang Wensheng]

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;
+		}
 
+		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 -next v3 2/2] watchdog: Clean up error handlings

[Wang Wensheng]

Clean up the repeated error handling process in function
__watchdog_register_device().

Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com>
---
 drivers/watchdog/watchdog_core.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 945ab38b14b8..4fa54a620ccd 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. */
@@ -270,9 +268,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		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;
+			ret = -EINVAL;
+			goto dev_unregister;
 		}
 
 		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
@@ -280,9 +277,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		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;
 		}
 	}
 
@@ -296,6 +291,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;
 }
 
 /**
-- 
2.25.0

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

[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

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

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

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