Re: [PATCH] watchdog: core: add option to make timeouts read-only

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5006 bytes --]

On Tue, Mar 27, 2018 at 03:37:04AM -0700, Guenter Roeck wrote:
> On 03/27/2018 01:09 AM, Marcus Folkesson wrote:
> > Some systems may not allow applications to configure the watchdog
> > timer at all. This restriction is not limited to stop the watchdog,
> > but also change timeouts as well.
> > 
> > This adds a kernel parameter to disable the ability to change the
> > watchdog timouts from userspace.
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> 
> I don't believe that this would be a watchdog-only problem. Those users probably

It is not, and I agree that the whole requirement is a bit fuzzy.
The requesters reference is a small system with a dedicaded MCU, and tries to
apply the same requirements on a Linux system without really understand
what it means.

> want lots of other things read-only. If they don't, and if this is really a watchdog
> specific request, I don't think they know what they are talking about. Besides,
> one can bypass it by unloading/reloading the drivers, so I don't really get the
> point.
> 
> Are the requesters of this feature aware that, with the permissions necessary
> to change a watchdog timeout, it is possible to do lots of things, such as,
> say, reboot the system ? Or cause a crash ? Is that less critical ?

It is not possible to login to these systems. The requirement is, what I
can tell, only to restrict applications (which could be external)
to modify the timeout. Reboots and systems crashes seems to be okay.

Strain out a gnat but swallow a camel, I know.

> 
> I would _really_ want to see a more detailed case made than "some users want it"
> before agreeing to a change like this.

I guess my only case is "some users want it" for now.

The fact that I have seen such a formulated requirement in two
independent projects made me submit it upstreams.

If I overlook the paranoid requirement, I think it could be useful to
guarantee that the timeout set by bootloader (bootargs) or devicetree is
the value that is used and not overridden.

But again, it is something that "some users may want".

> 
> Guenter
> 

Best regards
Marcus Folkesson

> > ---
> >   drivers/watchdog/Kconfig        |  9 +++++++++
> >   drivers/watchdog/watchdog_dev.c | 12 ++++++++++--
> >   2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index aff773bcebdb..bcba48b5c88b 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -46,6 +46,15 @@ config WATCHDOG_NOWAYOUT
> >   	  get killed. If you say Y here, the watchdog cannot be stopped once
> >   	  it has been started.
> > +config WATCHDOG_TIMEOUT_READONLY
> > +	bool "Make timeouts read-only from userspace"
> > +	help
> > +	  Say Y here if you want the watchdog timeout/pretimeout to be read-only
> > +	  from userspace. This requires that the timeout is configured before
> > +	  userspace takes over.
> > +
> > +	  Say N if you are unsure.
> > +
> >   config WATCHDOG_HANDLE_BOOT_ENABLED
> >   	bool "Update boot-enabled watchdog until userspace takes over"
> >   	default y
> > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > index ffbdc4642ea5..6064806a2a8d 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -87,6 +87,9 @@ static struct kthread_worker *watchdog_kworker;
> >   static bool handle_boot_enabled =
> >   	IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
> > +static bool timeout_is_readonly =
> > +	IS_ENABLED(CONFIG_WATCHDOG_TIMEOUT_READONLY);
> > +
> >   static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >   {
> >   	/* All variables in milli-seconds */
> > @@ -359,7 +362,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
> >   {
> >   	int err = 0;
> > -	if (!(wdd->info->options & WDIOF_SETTIMEOUT))
> > +	if (!(wdd->info->options & WDIOF_SETTIMEOUT) || timeout_is_readonly)
> >   		return -EOPNOTSUPP;
> >   	if (watchdog_timeout_invalid(wdd, timeout))
> > @@ -390,7 +393,7 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
> >   {
> >   	int err = 0;
> > -	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> > +	if (!(wdd->info->options & WDIOF_PRETIMEOUT) || timeout_is_readonly)
> >   		return -EOPNOTSUPP;
> >   	if (watchdog_pretimeout_invalid(wdd, timeout))
> > @@ -1181,3 +1184,8 @@ module_param(handle_boot_enabled, bool, 0444);
> >   MODULE_PARM_DESC(handle_boot_enabled,
> >   	"Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
> >   	__MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
> > +
> > +module_param(timeout_is_readonly, bool, 0444);
> > +MODULE_PARM_DESC(timeout_is_readonly,
> > +	"Watchdog timeouts is readonly from userspace (default="
> > +	__MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_TIMEOUT_READONLY)) ")");
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]