Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Nov 2022 02:36:02 +0100
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] watchdog: core: don't reset KEEPALIVEPING through
 sysfs
Message-ID: <63e88f05-0431-429b-8285-7dcd7e5782fe@t-8ch.de>
References: <20221127154559.80899-1-linux@weissschuh.net>
 <6ee65abe-8620-956d-a4a2-4ec5ec6257a5@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6ee65abe-8620-956d-a4a2-4ec5ec6257a5@roeck-us.net>
Jabber-ID: thomas@t-8ch.de
Precedence: bulk

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