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