diff mbox series

[RFC] watchdog: core: don't reset KEEPALIVEPING through sysfs

Message ID 20221127154559.80899-1-linux@weissschuh.net (mailing list archive)
State Rejected
Headers show
Series [RFC] watchdog: core: don't reset KEEPALIVEPING through sysfs | expand

Commit Message

Thomas Weißschuh Nov. 27, 2022, 3:45 p.m. UTC 
Reading the watchdog status via ioctl(WDIOC_GETSTATUS) or sysfs will
reset the status bit KEEPALIVEPING.

This is done so an application can validate that the watchdog was pinged
since the last read of the status.

For the ioctl-based interface this is fine as only one application can
manage a watchdog interface at a time, so it can properly handle this
implicit state modification.

The sysfs "status" file however is world-readable. This means that the
watchdog state can be modified by any other unprivileged process on the
system.

As the sysfs interface can also not be used to set this bit, let's
remove the capability to clear it.

Fixes: 90b826f17a4e ("watchdog: Implement status function in watchdog core")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---

I was not able to find an application (besides wdctl) that uses this
bit. But if applications are to use it, it should probably be reliable.

Other possible solutions I can think of:
* Only reset the bit when the file opened privileged
* Only reset the bit when the file opened writable

Instead of using a status bit to check if the watchdog was pinged it
would probably be more robust to use a sequence counter or timestamp.
Especially as it seems more operations are being exposed over sysfs over
time.

I'm not sure it's worth it though.
---
 Documentation/ABI/testing/sysfs-class-watchdog |  3 ++-
 drivers/watchdog/watchdog_dev.c                | 13 +++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)


base-commit: faf68e3523c21d07c5f7fdabd0daf6301ff8db3f

Comments

Guenter Roeck Nov. 27, 2022, 4:47 p.m. UTC | #1 
On 11/27/22 07:45, Thomas Weißschuh wrote:
> Reading the watchdog status via ioctl(WDIOC_GETSTATUS) or sysfs will
> reset the status bit KEEPALIVEPING.
> 
> This is done so an application can validate that the watchdog was pinged
> since the last read of the status.
> 
> For the ioctl-based interface this is fine as only one application can
> manage a watchdog interface at a time, so it can properly handle this
> implicit state modification.
> 
> The sysfs "status" file however is world-readable. This means that the
> watchdog state can be modified by any other unprivileged process on the
> system.
> 
> As the sysfs interface can also not be used to set this bit, let's
> remove the capability to clear it.
> 
> Fixes: 90b826f17a4e ("watchdog: Implement status function in watchdog core")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> ---
> 
> I was not able to find an application (besides wdctl) that uses this
> bit. But if applications are to use it, it should probably be reliable.
> 
> Other possible solutions I can think of:
> * Only reset the bit when the file opened privileged
> * Only reset the bit when the file opened writable
> 

All suggested solutions would be changing the ABI, which would be problematic.

As you have proposed elsewhere, it is possible for applications to chose where
to get the status from: sysfs or ioctl. It may well be that there is some
application out there which uses the sysfs attribute to read the status
and the ioctl otherwise. That would be odd, but possible.

Also, I can not imagine a real world use except for maybe reading the status
bit using sysfs from one application and checking if the watchdog demon actually
pinged it as it should ... but that is exactly what you are trying to disable
here.

Overall, this is probably the least valuable status bit. Any application should
know if it pinged the watchdog or not.

So: What real world problem have you observed that you are trying to solve ?
If there is no real observed problem, we should not entertain changing the ABI.
Actually, we can not change the ABI We would have to add another non-invasive
attribute that doesn't change the status when read. That should really be
worth the trouble.

Guenter

> Instead of using a status bit to check if the watchdog was pinged it
> would probably be more robust to use a sequence counter or timestamp.
> Especially as it seems more operations are being exposed over sysfs over
> time.
> 
> I'm not sure it's worth it though.
> ---
>   Documentation/ABI/testing/sysfs-class-watchdog |  3 ++-
>   drivers/watchdog/watchdog_dev.c                | 13 +++++++++----
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog
> index 585caecda3a5..182a8a9e530e 100644
> --- a/Documentation/ABI/testing/sysfs-class-watchdog
> +++ b/Documentation/ABI/testing/sysfs-class-watchdog
> @@ -38,7 +38,8 @@ Contact:	Wim Van Sebroeck <wim@iguana.be>
>   Description:
>   		It is a read only file. It contains watchdog device's
>   		internal status bits. It is equivalent to WDIOC_GETSTATUS
> -		of ioctl interface.
> +		of ioctl interface, except that the status bit
> +		WDIOF_KEEPALIVEPING is not reset.
>   
>   What:		/sys/class/watchdog/watchdogn/timeleft
>   Date:		August 2015
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 55574ed42504..7e3e964c4d6f 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -320,13 +320,15 @@ static int watchdog_stop(struct watchdog_device *wdd)
>   /*
>    * watchdog_get_status - wrapper to get the watchdog status
>    * @wdd: The watchdog device to get the status from
> + * @reset_keepaliveping: Reset the status bit _WDOG_KEEPALIVE
>    *
>    * Get the watchdog's status flags.
>    * The caller must hold wd_data->lock.
>    *
>    * Return: watchdog's status flags.
>    */
> -static unsigned int watchdog_get_status(struct watchdog_device *wdd)
> +static unsigned int watchdog_get_status(struct watchdog_device *wdd,
> +					bool reset_keepaliveping)
>   {
>   	struct watchdog_core_data *wd_data = wdd->wd_data;
>   	unsigned int status;
> @@ -345,9 +347,12 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
>   	if (test_bit(_WDOG_ALLOW_RELEASE, &wd_data->status))
>   		status |= WDIOF_MAGICCLOSE;
>   
> -	if (test_and_clear_bit(_WDOG_KEEPALIVE, &wd_data->status))
> +	if (test_bit(_WDOG_KEEPALIVE, &wd_data->status))
>   		status |= WDIOF_KEEPALIVEPING;
>   
> +	if (reset_keepaliveping)
> +		clear_bit(_WDOG_KEEPALIVE, &wd_data->status);
> +
>   	if (IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
>   		status |= WDIOF_PRETIMEOUT;
>   
> @@ -476,7 +481,7 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>   	unsigned int status;
>   
>   	mutex_lock(&wd_data->lock);
> -	status = watchdog_get_status(wdd);
> +	status = watchdog_get_status(wdd, false);
>   	mutex_unlock(&wd_data->lock);
>   
>   	return sysfs_emit(buf, "0x%x\n", status);
> @@ -753,7 +758,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   			sizeof(struct watchdog_info)) ? -EFAULT : 0;
>   		break;
>   	case WDIOC_GETSTATUS:
> -		val = watchdog_get_status(wdd);
> +		val = watchdog_get_status(wdd, true);
>   		err = put_user(val, p);
>   		break;
>   	case WDIOC_GETBOOTSTATUS:
> 
> base-commit: faf68e3523c21d07c5f7fdabd0daf6301ff8db3f
Thomas Weißschuh Nov. 28, 2022, 1:36 a.m. UTC | #2 
On 2022-11-27 08:47-0800, Guenter Roeck wrote:
> On 11/27/22 07:45, Thomas Weißschuh wrote:
>> Reading the watchdog status via ioctl(WDIOC_GETSTATUS) or sysfs will
>> reset the status bit KEEPALIVEPING.
>> 
>> This is done so an application can validate that the watchdog was pinged
>> since the last read of the status.
>> 
>> For the ioctl-based interface this is fine as only one application can
>> manage a watchdog interface at a time, so it can properly handle this
>> implicit state modification.
>> 
>> The sysfs "status" file however is world-readable. This means that the
>> watchdog state can be modified by any other unprivileged process on the
>> system.
>> 
>> As the sysfs interface can also not be used to set this bit, let's
>> remove the capability to clear it.
>> 
>> Fixes: 90b826f17a4e ("watchdog: Implement status function in watchdog core")
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> 
>> ---
>> 
>> I was not able to find an application (besides wdctl) that uses this
>> bit. But if applications are to use it, it should probably be reliable.
>> 
>> Other possible solutions I can think of:
>> * Only reset the bit when the file opened privileged
>> * Only reset the bit when the file opened writable
>> 
> 
> All suggested solutions would be changing the ABI, which would be problematic.
> 
> As you have proposed elsewhere, it is possible for applications to chose where
> to get the status from: sysfs or ioctl. It may well be that there is some
> application out there which uses the sysfs attribute to read the status
> and the ioctl otherwise. That would be odd, but possible.
> 
> Also, I can not imagine a real world use except for maybe reading the status
> bit using sysfs from one application and checking if the watchdog demon actually
> pinged it as it should ... but that is exactly what you are trying to disable
> here.

Good point.

> Overall, this is probably the least valuable status bit. Any application should
> know if it pinged the watchdog or not.
> 
> So: What real world problem have you observed that you are trying to solve ?
> If there is no real observed problem, we should not entertain changing the ABI.
> Actually, we can not change the ABI We would have to add another non-invasive
> attribute that doesn't change the status when read. That should really be
> worth the trouble.

I have not observed a real problem, only weird behavior while working on wdctl.
From this I figured that this could be a problem in case another, malicious or broken
process accesses the state file and resets the status bit.

To make you aware of this observation I sent the RFC patch.

If you think it's not a problem worth fixing this patch can be dropped.

> Guenter
> 
>> Instead of using a status bit to check if the watchdog was pinged it
>> would probably be more robust to use a sequence counter or timestamp.
>> Especially as it seems more operations are being exposed over sysfs over
>> time.
>> 
>> I'm not sure it's worth it though.
>> ---
>>   Documentation/ABI/testing/sysfs-class-watchdog |  3 ++-
>>   drivers/watchdog/watchdog_dev.c                | 13 +++++++++----
>>   2 files changed, 11 insertions(+), 5 deletions(-)
>> 
>> [..]
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog
index 585caecda3a5..182a8a9e530e 100644
--- a/Documentation/ABI/testing/sysfs-class-watchdog
+++ b/Documentation/ABI/testing/sysfs-class-watchdog
@@ -38,7 +38,8 @@  Contact:	Wim Van Sebroeck <wim@iguana.be>
 Description:
 		It is a read only file. It contains watchdog device's
 		internal status bits. It is equivalent to WDIOC_GETSTATUS
-		of ioctl interface.
+		of ioctl interface, except that the status bit
+		WDIOF_KEEPALIVEPING is not reset.
 
 What:		/sys/class/watchdog/watchdogn/timeleft
 Date:		August 2015
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 55574ed42504..7e3e964c4d6f 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -320,13 +320,15 @@  static int watchdog_stop(struct watchdog_device *wdd)
 /*
  * watchdog_get_status - wrapper to get the watchdog status
  * @wdd: The watchdog device to get the status from
+ * @reset_keepaliveping: Reset the status bit _WDOG_KEEPALIVE
  *
  * Get the watchdog's status flags.
  * The caller must hold wd_data->lock.
  *
  * Return: watchdog's status flags.
  */
-static unsigned int watchdog_get_status(struct watchdog_device *wdd)
+static unsigned int watchdog_get_status(struct watchdog_device *wdd,
+					bool reset_keepaliveping)
 {
 	struct watchdog_core_data *wd_data = wdd->wd_data;
 	unsigned int status;
@@ -345,9 +347,12 @@  static unsigned int watchdog_get_status(struct watchdog_device *wdd)
 	if (test_bit(_WDOG_ALLOW_RELEASE, &wd_data->status))
 		status |= WDIOF_MAGICCLOSE;
 
-	if (test_and_clear_bit(_WDOG_KEEPALIVE, &wd_data->status))
+	if (test_bit(_WDOG_KEEPALIVE, &wd_data->status))
 		status |= WDIOF_KEEPALIVEPING;
 
+	if (reset_keepaliveping)
+		clear_bit(_WDOG_KEEPALIVE, &wd_data->status);
+
 	if (IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
 		status |= WDIOF_PRETIMEOUT;
 
@@ -476,7 +481,7 @@  static ssize_t status_show(struct device *dev, struct device_attribute *attr,
 	unsigned int status;
 
 	mutex_lock(&wd_data->lock);
-	status = watchdog_get_status(wdd);
+	status = watchdog_get_status(wdd, false);
 	mutex_unlock(&wd_data->lock);
 
 	return sysfs_emit(buf, "0x%x\n", status);
@@ -753,7 +758,7 @@  static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			sizeof(struct watchdog_info)) ? -EFAULT : 0;
 		break;
 	case WDIOC_GETSTATUS:
-		val = watchdog_get_status(wdd);
+		val = watchdog_get_status(wdd, true);
 		err = put_user(val, p);
 		break;
 	case WDIOC_GETBOOTSTATUS: