[PATCH] watchdog: Device must be opened for writing

[PATCH] watchdog: Device must be opened for writing

[Stanislav German-Evtushenko]

If userspace opens the watchdog device self-feeding stops. Sometimes
opening the device happens by accident, e.g. by mistakenly running grep
recursively in a wrong directory which leads to the server being reset.

Watchdog device does not handle read operation therefore the issue can be
prevented by requiring the device to be opened for writing:

- Prevent opening the device without FMODE_WRITE

Signed-off-by: Stanislav German-Evtushenko <ginermail@gmail.com>
---
 drivers/watchdog/watchdog_dev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 3bab32485273..28b88542a4d0 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -835,6 +835,10 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	bool hw_running;
 	int err;
 
+	/* Must be open for writing */
+	if (!(file->f_mode & FMODE_WRITE))
+		return -EINVAL;
+
 	/* Get the corresponding watchdog device */
 	if (imajor(inode) == MISC_MAJOR)
 		wd_data = old_wd_data;

base-commit: cf813c67d9619fd474c785698cbed543b94209dd
-- 
2.25.1

Re: [PATCH] watchdog: Device must be opened for writing

[Guenter Roeck]

On Sun, Aug 15, 2021 at 04:13:45AM +0900, Stanislav German-Evtushenko wrote:
> If userspace opens the watchdog device self-feeding stops. Sometimes
> opening the device happens by accident, e.g. by mistakenly running grep
> recursively in a wrong directory which leads to the server being reset.
> 
> Watchdog device does not handle read operation therefore the issue can be
> prevented by requiring the device to be opened for writing:
> 
> - Prevent opening the device without FMODE_WRITE
> 
> Signed-off-by: Stanislav German-Evtushenko <ginermail@gmail.com>

NACK. That would be a major undocumented ABI change. Opening /dev/watchdog
for reading can be and is used today to test a watchdog.

Guenter

> ---
>  drivers/watchdog/watchdog_dev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 3bab32485273..28b88542a4d0 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -835,6 +835,10 @@ static int watchdog_open(struct inode *inode, struct file *file)
>  	bool hw_running;
>  	int err;
>  
> +	/* Must be open for writing */
> +	if (!(file->f_mode & FMODE_WRITE))
> +		return -EINVAL;
> +
>  	/* Get the corresponding watchdog device */
>  	if (imajor(inode) == MISC_MAJOR)
>  		wd_data = old_wd_data;
> 
> base-commit: cf813c67d9619fd474c785698cbed543b94209dd
> -- 
> 2.25.1
> 

Re: [PATCH] watchdog: Device must be opened for writing

[Guenter Roeck]

On 8/14/21 5:19 PM, Stanislav German-Evtushenko wrote:
> On Saturday, August 14, 2021, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
>  > On Sun, Aug 15, 2021 at 04:13:45AM +0900, Stanislav German-Evtushenko wrote:
>  >> If userspace opens the watchdog device self-feeding stops. Sometimes
>  >> opening the device happens by accident, e.g. by mistakenly running grep
>  >> recursively in a wrong directory which leads to the server being reset.
>  >>
>  >> Watchdog device does not handle read operation therefore the issue can be
>  >> prevented by requiring the device to be opened for writing:
>  >>
>  >> - Prevent opening the device without FMODE_WRITE
>  >>
>  >> Signed-off-by: Stanislav German-Evtushenko <ginermail@gmail.com <mailto:ginermail@gmail.com>>
>  >
>  > NACK. That would be a major undocumented ABI change. Opening /dev/watchdog
>  > for reading can be and is used today to test a watchdog.
>  >
>  > Guenter
>  >
> 
> I see. This is unfortunate.
> 
> In this case I'll try to find the right place in the documentation and make it more explicit unless it is already there and I've overlooked.
> 
No. That isn't the point. This is used and must not be changed.
Again, people (including me( are _using_ this to test watchdogs.
We are not going to disable that because some people are not
careful when executing commands as root.

Guenter

Re: [PATCH] watchdog: Device must be opened for writing

[Stanislav German-Evtushenko]

On 8/15/21, Guenter Roeck <linux@roeck-us.net> wrote:
> On 8/14/21 5:19 PM, Stanislav German-Evtushenko wrote:
>> On Saturday, August 14, 2021, Guenter Roeck <linux@roeck-us.net
>> <mailto:linux@roeck-us.net>> wrote:
>>  > On Sun, Aug 15, 2021 at 04:13:45AM +0900, Stanislav German-Evtushenko
>> wrote:
>>  >> If userspace opens the watchdog device self-feeding stops. Sometimes
>>  >> opening the device happens by accident, e.g. by mistakenly running
>> grep
>>  >> recursively in a wrong directory which leads to the server being
>> reset.
>>  >>
>>  >> Watchdog device does not handle read operation therefore the issue can
>> be
>>  >> prevented by requiring the device to be opened for writing:
>>  >>
>>  >> - Prevent opening the device without FMODE_WRITE
>>  >>
>>  >> Signed-off-by: Stanislav German-Evtushenko <ginermail@gmail.com
>> <mailto:ginermail@gmail.com>>
>>  >
>>  > NACK. That would be a major undocumented ABI change. Opening
>> /dev/watchdog
>>  > for reading can be and is used today to test a watchdog.
>>  >
>>  > Guenter
>>  >
>>
>> I see. This is unfortunate.
>>
>> In this case I'll try to find the right place in the documentation and
>> make it more explicit unless it is already there and I've overlooked.
>>
> No. That isn't the point. This is used and must not be changed.
> Again, people (including me( are _using_ this to test watchdogs.
> We are not going to disable that because some people are not
> careful when executing commands as root.
>
> Guenter
>

I mean I would describe the existing behavior as this is not obvious.

As for testing, opening for writing is not harder than for reading but
I understand that it would be a breaking change now.