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

["Thomas Weißschuh" <linux@weissschuh.net>]

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(-)
>> 
>> [..]