watchdog: core: add option to make timeouts read-only
cover watchdog: core: add option to make timeouts read-only
I have encountered at least two projects (automotive) where there is a strong demand for no-one to touch the configuration of the watchdog timer once started.
File permissions and other restrictions has not been enough to satisfy the requirement.
There you have some background information for this patch.
Cheers,
Marcus Folkesson
PATCH 1/1 watchdog: core: add option to make timeouts read-only
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.
--- drivers/watchdog/Kconfig | 9 +++++++++ drivers/watchdog/watchdog_dev.c | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)
drivers/watchdog/Kconfig +9 -0
@@ -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
drivers/watchdog/watchdog_dev.c +10 -2
@@ -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)) ")");
Signature · 1 line
2.16.2
Guenter Roeck
maintainerQuoted 8 lines · Marcus Folkesson
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 <[email protected]>
I don't believe that this would be a watchdog-only problem. Those users probably 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 ?
I would _really_ want to see a more detailed case made than "some users want it" before agreeing to a change like this.
Guenter
Quoted 67 lines
--- drivers/watchdog/Kconfig | 9 +++++++++ drivers/watchdog/watchdog_dev.c | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)drivers/watchdog/Kconfig +9
-0@@ -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 ydrivers/watchdog/watchdog_dev.c +10
-2@@ -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)) ")");
Quoted 11 lines · Guenter Roeck
Quoted 8 lines · Marcus Folkesson
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 <[email protected]>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.
Quoted 8 lines
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.
Quoted 3 lines
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".
Quoted 3 lines
Guenter
Best regards
Marcus Folkesson
Quoted 62 lines
Quoted 61 lines
--- drivers/watchdog/Kconfig | 9 +++++++++ drivers/watchdog/watchdog_dev.c | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)drivers/watchdog/Kconfig +9
-0@@ -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 ydrivers/watchdog/watchdog_dev.c +10
-2@@ -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)) ")");
Guenter Roeck
maintainerQuoted 34 lines · Marcus Folkesson
Quoted 11 lines · Guenter Roeck
Quoted 8 lines · Marcus Folkesson
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 <[email protected]>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.
Quoted 8 lines
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.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
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.
Sorry, but this doesn't make any sense. Anyone who really wants to defeat such a "security" mechanism can easily do it.
Just imagine the context. Changing the timeout must be done by the watchdog daemon. If that can be modified, it was loaded from insecure storage. If it can not be modified, its configuration was loaded from insecure storage. Both is _bad_. On top of that, if it is possible to run insecure applications, those might as well do direct i2c or mmio access and program the watchdog directly. In other words, restricting the API to change the watchdog timeout does not improve system security at all. At best it would hide its insecurity, and it would be a pure "feel good" measure without any real value.
I would really like to see a complete security evaluation of the projects you mention. They must be full of security holes, or they would not even think about requesting something like this. Automotive, you said ? Outch.
Quoted 6 lines
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".
The simple answer to that is that they should not do it if they don't want it. It does not make sense to define kernel configuration options for policy decisions like this.
In summary, NACK.
Guenter
Quoted 69 lines
Quoted 3 lines
Guenter
Best regards
Marcus FolkessonQuoted 62 lines
Quoted 61 lines
--- drivers/watchdog/Kconfig | 9 +++++++++ drivers/watchdog/watchdog_dev.c | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)drivers/watchdog/Kconfig +9
-0@@ -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 ydrivers/watchdog/watchdog_dev.c +10
-2@@ -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)) ")");