| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <168af019-70d1-3237-dc9c-56a82beb5990@roeck-us.net>
Date: Thu, 16 Jun 2022 06:36:26 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Daniel Bristot de Oliveira <bristot@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>,
Jonathan Corbet <corbet@....net>,
Ingo Molnar <mingo@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Will Deacon <will@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Marco Elver <elver@...gle.com>,
Dmitry Vyukov <dvyukov@...gle.com>,
"Paul E. McKenney" <paulmck@...nel.org>,
Shuah Khan <skhan@...uxfoundation.org>,
Gabriele Paoloni <gpaoloni@...hat.com>,
Juri Lelli <juri.lelli@...hat.com>,
Clark Williams <williams@...hat.com>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-trace-devel@...r.kernel.org
Subject: Re: [PATCH V4 18/20] rv/monitor: Add safe watchdog monitor
On 6/16/22 01:45, Daniel Bristot de Oliveira wrote:
> The watchdog is an essential building block for the usage of Linux in
> safety-critical systems because it allows the system to be monitored from
> an external element - the watchdog hardware, acting as a safety-monitor.
>
> A user-space application controls the watchdog device via the watchdog
> interface. This application, hereafter safety_app, enables the watchdog
> and periodically pets the watchdog upon correct completion of the safety
> related processing.
>
> If the safety_app, for any reason, stops pinging the watchdog,
> the watchdog hardware can set the system in a fail-safe state. For
> example, shutting the system down.
>
> Given the importance of the safety_app / watchdog hardware couple,
> the interaction between these software pieces also needs some
> sort of monitoring. In other words, "who monitors the monitor?"
>
> The safe watchdog (safe_wtd) RV monitor monitors the interaction between
> the safety_app and the watchdog device, enforcing the correct sequence of
> events that leads the system to a safe state.
>
> Furthermore, the safety_app can monitor the RV monitor by collecting the
> events generated by the RV monitor itself via tracing interface. In this way,
> closing the monitoring loop with the safety_app.
>
> To reach a safe state, the safe_wtd RV monitor requires the
> safety_app to:
>
> - Open the watchdog device
> - Start the watchdog
> - Set a timeout
> - ping at least once
>
> The RV monitor also avoids some undesired actions. For example, to have
> other threads to touch the watchdog.
>
> The monitor also has a set of options, enabled via kernel command
> line/module options. They are:
>
> - watchdog_id: the device id to monitor (default 0).
> - dont_stop: once enabled, do not allow the RV monitor to be stopped
> (default off);
> - safe_timeout: define a maximum safe value that an user-space
> application can set as the watchdog timeout
> (default unlimited).
> - check_timeout: After every ping, check if the time left in the
> watchdog is less than or equal to the last timeout set
> for the watchdog. It only works for watchdog devices that
> provide the get_timeleft() function (default off).
>
> For further information, please refer to:
> Documentation/trace/rv/watchdog-monitor.rst
>
> The monitor specification was developed together with Gabriele Paoloni,
> in the context of the Linux Foundation Elisa Project.
>
> Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>
> Cc: Guenter Roeck <linux@...ck-us.net>
> Cc: Jonathan Corbet <corbet@....net>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Will Deacon <will@...nel.org>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Marco Elver <elver@...gle.com>
> Cc: Dmitry Vyukov <dvyukov@...gle.com>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Shuah Khan <skhan@...uxfoundation.org>
> Cc: Gabriele Paoloni <gpaoloni@...hat.com>
> Cc: Juri Lelli <juri.lelli@...hat.com>
> Cc: Clark Williams <williams@...hat.com>
> Cc: linux-doc@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-trace-devel@...r.kernel.org
> Signed-off-by: Daniel Bristot de Oliveira <bristot@...nel.org>
> ---
> include/trace/events/rv.h | 11 +
> kernel/trace/rv/Kconfig | 10 +
> kernel/trace/rv/Makefile | 1 +
> kernel/trace/rv/monitors/safe_wtd/safe_wtd.c | 300 +++++++++++++++++++
> kernel/trace/rv/monitors/safe_wtd/safe_wtd.h | 84 ++++++
> 5 files changed, 406 insertions(+)
> create mode 100644 kernel/trace/rv/monitors/safe_wtd/safe_wtd.c
> create mode 100644 kernel/trace/rv/monitors/safe_wtd/safe_wtd.h
>
> diff --git a/include/trace/events/rv.h b/include/trace/events/rv.h
> index 00f11a8dac3b..895eb3435ed7 100644
> --- a/include/trace/events/rv.h
> +++ b/include/trace/events/rv.h
> @@ -66,6 +66,17 @@ DEFINE_EVENT(error_da_monitor, error_wip,
> TP_PROTO(char *state, char *event),
> TP_ARGS(state, event));
> #endif /* CONFIG_RV_MON_WIP */
> +
> +#ifdef CONFIG_RV_MON_SAFE_WTD
> +DEFINE_EVENT(event_da_monitor, event_safe_wtd,
> + TP_PROTO(char *state, char *event, char *next_state, bool safe),
> + TP_ARGS(state, event, next_state, safe));
> +
> +DEFINE_EVENT(error_da_monitor, error_safe_wtd,
> + TP_PROTO(char *state, char *event),
> + TP_ARGS(state, event));
> +#endif /* CONFIG_RV_MON_SAFE_WTD */
> +
> #endif /* CONFIG_DA_MON_EVENTS_IMPLICIT */
>
> #ifdef CONFIG_DA_MON_EVENTS_ID
> diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
> index 21f03fb3101a..b14ae63e792b 100644
> --- a/kernel/trace/rv/Kconfig
> +++ b/kernel/trace/rv/Kconfig
> @@ -45,6 +45,16 @@ config RV_MON_WWNR
> illustrates the usage of per-task monitor. The model is
> broken on purpose: it serves to test reactors.
>
> +config RV_MON_SAFE_WTD
> + select DA_MON_EVENTS_IMPLICIT
> + bool "Safety watchdog"
> + help
> + Enable safe_wtd, this monitor observes the interaction
> + between a user-space safety monitor and a watchdog device.
> +
> + For futher information see:
> + Documentation/trace/rv/safety-monitor.rst
> +
> config RV_REACTORS
> bool "Runtime verification reactors"
> default y if RV
> diff --git a/kernel/trace/rv/Makefile b/kernel/trace/rv/Makefile
> index 963d14875b45..904db96c7eae 100644
> --- a/kernel/trace/rv/Makefile
> +++ b/kernel/trace/rv/Makefile
> @@ -3,6 +3,7 @@
> obj-$(CONFIG_RV) += rv.o
> obj-$(CONFIG_RV_MON_WIP) += monitors/wip/wip.o
> obj-$(CONFIG_RV_MON_WWNR) += monitors/wwnr/wwnr.o
> +obj-$(CONFIG_RV_MON_SAFE_WTD) += monitors/safe_wtd/safe_wtd.o
> obj-$(CONFIG_RV_REACTORS) += rv_reactors.o
> obj-$(CONFIG_RV_REACT_PRINTK) += reactor_printk.o
> obj-$(CONFIG_RV_REACT_PANIC) += reactor_panic.o
> diff --git a/kernel/trace/rv/monitors/safe_wtd/safe_wtd.c b/kernel/trace/rv/monitors/safe_wtd/safe_wtd.c
> new file mode 100644
> index 000000000000..9856e0770d0d
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/safe_wtd/safe_wtd.c
> @@ -0,0 +1,300 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/ftrace.h>
> +#include <linux/tracepoint.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/rv.h>
> +#include <rv/instrumentation.h>
> +#include <rv/da_monitor.h>
> +
> +#include <linux/watchdog.h>
> +#include <linux/moduleparam.h>
> +
> +#include <trace/events/rv.h>
> +#include <trace/events/watchdog.h>
> +
> +#define MODULE_NAME "safe_wtd"
> +
> +/*
> + * This is the self-generated part of the monitor. Generally, there is no need
> + * to touch this section.
> + */
> +#include "safe_wtd.h"
> +
> +/*
> + * Declare the deterministic automata monitor.
> + *
> + * The rv monitor reference is needed for the monitor declaration.
> + */
> +struct rv_monitor rv_safe_wtd;
> +DECLARE_DA_MON_GLOBAL(safe_wtd, char);
> +
> +/*
> + * custom: safe_timeout is the maximum value a watchdog monitor
> + * can set. This value is registered here to duplicate the information.
> + * In this way, a miss-behaving monitor can be detected.
> + */
> +static int safe_timeout = ~0;
> +module_param(safe_timeout, int, 0444);
> +
> +/*
> + * custom: if check_timeout is set, the monitor will check if the time left
> + * in the watchdog is less than or equals to the last safe timeout set by
> + * user-space. This check is done after each ping. In this way, if any
> + * code by-passed the watchdog dev interface setting a higher (so unsafe)
> + * timeout, this monitor will catch the side effect and react.
> + */
> +static int last_timeout_set = 0;
> +static int check_timeout = 0;
> +module_param(check_timeout, int, 0444);
> +
> +/*
> + * custom: if dont_stop is set the monitor will react if stopped.
> + */
> +static int dont_stop = 0;
> +module_param(dont_stop, int, 0444);
> +
> +/*
> + * custom: there are some states that are kept after the watchdog is closed.
> + * For example, the nowayout state.
> + *
> + * Thus, the RV monitor needs to keep track of these states after a start/stop
> + * of the RV monitor itself, and should not reset after each restart - keeping the
> + * know state until the system shutdown.
> + *
> + * If for an unknown reason an RV monitor would like to reset the RV monitor at each
> + * RV monitor start, set it to one.
> + */
> +static int reset_on_restart = 0;
> +module_param(reset_on_restart, int, 0444);
> +
> +/*
> + * open_pid takes note of the first thread that opened the watchdog.
> + *
> + * Any other thread that generates an event will cause an "other_threads"
> + * event in the monitor.
> + */
> +static int open_pid = 0;
Userspace could open a watchdog, create a child process, and handle it
from the child. That is perfectly valid.
> +
> +/*
> + * watchdog_id: the watchdog to monitor
> + */
> +static int watchdog_id = 0;
> +module_param(watchdog_id, int, 0444);
Limiting the watcher to a single watchdog sounds less than perfect.
What if the system supports more than one, more than one is enabled,
and the non-monitored watchdog misbehaves ?
> +
> +static void handle_nowayout(void *data, struct watchdog_device *wdd)
> +{
> + if (wdd->id != watchdog_id)
> + return;
> +
> + da_handle_init_run_event_safe_wtd(nowayout_safe_wtd);
> +}
> +
> +static void handle_close(void *data, struct watchdog_device *wdd)
> +{
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + } else {
> + da_handle_event_safe_wtd(close_safe_wtd);
> + open_pid = 0;
> + }
> +}
> +
> +static void handle_open(void *data, struct watchdog_device *wdd)
> +{
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + } else {
> + da_handle_init_run_event_safe_wtd(open_safe_wtd);
> + open_pid = current->pid;
> + }
> +}
> +
> +static void blocked_events(void *data, struct watchdog_device *wdd)
> +{
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + return;
> + }
> + da_handle_event_safe_wtd(other_threads_safe_wtd);
> +}
> +
> +static void blocked_events_timeout(void *data, struct watchdog_device *wdd, u64 timeout)
> +{
> + blocked_events(data, wdd);
> +}
> +
> +static void handle_ping(void *data, struct watchdog_device *wdd)
> +{
> + char msg[128];
> + unsigned int timeout;
> +
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + return;
> + }
> +
> + da_handle_event_safe_wtd(ping_safe_wtd);
> +
> + if (!check_timeout)
> + return;
> +
> + if (wdd->ops->get_timeleft) {
> + timeout = wdd->ops->get_timeleft(wdd);
> + if (timeout > last_timeout_set) {
> + snprintf(msg, 128,
> + "watchdog timeout is %u > than previously set (%d)\n",
> + timeout, last_timeout_set);
> + cond_react(msg);
> + }
> + } else {
> + snprintf(msg, 128, "error getting timeout: option not supported\n");
This is not an error. The get_timeleft callback is optional.
> + cond_react(msg);
> + }
> +}
> +
> +static void handle_set_safe_timeout(void *data, struct watchdog_device *wdd, u64 timeout)
> +{
> + char msg[128];
> +
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + return;
> + }
> +
> + da_handle_event_safe_wtd(set_safe_timeout_safe_wtd);
> +
> + if (timeout > safe_timeout) {
> + snprintf(msg, 128, "set safety timeout is too high: %d", (int) timeout);
> + cond_react(msg);
> + }
> +
> + if (check_timeout)
> + last_timeout_set = timeout;
> +}
> +
> +static void handle_start(void *data, struct watchdog_device *wdd)
> +{
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + return;
> + }
> +
> + da_handle_event_safe_wtd(start_safe_wtd);
> +}
> +
> +static void handle_stop(void *data, struct watchdog_device *wdd)
> +{
> + if (wdd->id != watchdog_id)
> + return;
> +
> + if (open_pid && current->pid != open_pid) {
> + da_handle_init_run_event_safe_wtd(other_threads_safe_wtd);
> + return;
> + }
> +
> + da_handle_event_safe_wtd(stop_safe_wtd);
> +}
> +
> +static int mon_started = 0;
> +
> +static int start_safe_wtd_monitor(void)
> +{
> + int retval;
> +
> + if (!mon_started || reset_on_restart) {
> + retval = da_monitor_init_safe_wtd();
> + if (retval)
> + return retval;
> +
> + mon_started = 1;
> + }
> +
> + rv_attach_trace_probe("safe_wtd", watchdog_close, handle_close);
> + rv_attach_trace_probe("safe_wtd", watchdog_nowayout, handle_nowayout);
> + rv_attach_trace_probe("safe_wtd", watchdog_open, handle_open);
> + rv_attach_trace_probe("safe_wtd", watchdog_ping, handle_ping);
> + rv_attach_trace_probe("safe_wtd", watchdog_set_timeout, handle_set_safe_timeout);
> + rv_attach_trace_probe("safe_wtd", watchdog_start, handle_start);
> + rv_attach_trace_probe("safe_wtd", watchdog_stop, handle_stop);
> + rv_attach_trace_probe("safe_wtd", watchdog_set_keep_alive, blocked_events_timeout);
> + rv_attach_trace_probe("safe_wtd", watchdog_keep_alive, blocked_events);
> + rv_attach_trace_probe("safe_wtd", watchdog_set_pretimeout, blocked_events_timeout);
> + rv_attach_trace_probe("safe_wtd", watchdog_pretimeout, blocked_events);
> +
> + return 0;
> +}
> +
> +static void stop_safe_wtd_monitor(void)
> +{
> + if (dont_stop)
> + cond_react("dont_stop safe_wtd is set.");
> +
> + rv_safe_wtd.enabled = 0;
> +
> + rv_detach_trace_probe("safe_wtd", watchdog_close, handle_close);
> + rv_detach_trace_probe("safe_wtd", watchdog_nowayout, handle_nowayout);
> + rv_detach_trace_probe("safe_wtd", watchdog_open, handle_open);
> + rv_detach_trace_probe("safe_wtd", watchdog_ping, handle_ping);
> + rv_detach_trace_probe("safe_wtd", watchdog_set_timeout, handle_set_safe_timeout);
> + rv_detach_trace_probe("safe_wtd", watchdog_start, handle_start);
> + rv_detach_trace_probe("safe_wtd", watchdog_stop, handle_stop);
> + rv_detach_trace_probe("safe_wtd", watchdog_set_keep_alive, blocked_events_timeout);
> + rv_detach_trace_probe("safe_wtd", watchdog_keep_alive, blocked_events);
> + rv_detach_trace_probe("safe_wtd", watchdog_set_pretimeout, blocked_events_timeout);
> + rv_detach_trace_probe("safe_wtd", watchdog_pretimeout, blocked_events);
> +
> + da_monitor_destroy_safe_wtd();
> +}
> +
> +/*
> + * This is the monitor register section.
> + */
> +struct rv_monitor rv_safe_wtd = {
> + .name = "safe_wtd",
> + .description = "A watchdog monitor guarding a safety monitor actions",
> + .start = start_safe_wtd_monitor,
> + .stop = stop_safe_wtd_monitor,
> + .reset = da_monitor_reset_all_safe_wtd,
> + .enabled = 0,
> +};
> +
> +int register_safe_wtd(void)
> +{
> + rv_register_monitor(&rv_safe_wtd);
> + return 0;
> +}
> +
> +void unregister_safe_wtd(void)
> +{
> + if (rv_safe_wtd.enabled)
> + stop_safe_wtd_monitor();
> +
> + rv_unregister_monitor(&rv_safe_wtd);
> +}
> +
> +module_init(register_safe_wtd);
> +module_exit(unregister_safe_wtd);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Daniel Bristot de Oliveira <bristot@...nel.org>");
> +MODULE_DESCRIPTION("Safe watchdog RV monitor");
> diff --git a/kernel/trace/rv/monitors/safe_wtd/safe_wtd.h b/kernel/trace/rv/monitors/safe_wtd/safe_wtd.h
> new file mode 100644
> index 000000000000..835c9d0979f6
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/safe_wtd/safe_wtd.h
> @@ -0,0 +1,84 @@
> +enum states_safe_wtd {
> + init_safe_wtd = 0,
> + closed_running_safe_wtd,
> + closed_running_nwo_safe_wtd,
> + nwo_safe_wtd,
> + opened_safe_wtd,
> + opened_nwo_safe_wtd,
> + reopened_safe_wtd,
> + safe_safe_wtd,
> + safe_nwo_safe_wtd,
> + set_safe_wtd,
> + set_nwo_safe_wtd,
> + started_safe_wtd,
> + started_nwo_safe_wtd,
> + stoped_safe_wtd,
> + state_max_safe_wtd
> +};
> +
> +enum events_safe_wtd {
> + close_safe_wtd = 0,
> + nowayout_safe_wtd,
> + open_safe_wtd,
> + other_threads_safe_wtd,
> + ping_safe_wtd,
> + set_safe_timeout_safe_wtd,
> + start_safe_wtd,
> + stop_safe_wtd,
> + event_max_safe_wtd
> +};
> +
> +struct automaton_safe_wtd {
> + char *state_names[state_max_safe_wtd];
> + char *event_names[event_max_safe_wtd];
> + char function[state_max_safe_wtd][event_max_safe_wtd];
> + char initial_state;
> + char final_states[state_max_safe_wtd];
> +};
> +
> +struct automaton_safe_wtd automaton_safe_wtd = {
> + .state_names = {
> + "init",
> + "closed_running",
> + "closed_running_nwo",
> + "nwo",
> + "opened",
> + "opened_nwo",
> + "reopened",
> + "safe",
> + "safe_nwo",
> + "set",
> + "set_nwo",
> + "started",
> + "started_nwo",
> + "stoped"
> + },
> + .event_names = {
> + "close",
> + "nowayout",
> + "open",
> + "other_threads",
> + "ping",
> + "set_safe_timeout",
> + "start",
> + "stop"
> + },
> + .function = {
> + { -1, nwo_safe_wtd, opened_safe_wtd, init_safe_wtd, -1, -1, -1, -1 },
> + { -1, closed_running_nwo_safe_wtd, reopened_safe_wtd, closed_running_safe_wtd, -1, -1, -1, -1 },
> + { -1, closed_running_nwo_safe_wtd, started_nwo_safe_wtd, closed_running_nwo_safe_wtd, -1, -1, -1, -1 },
> + { -1, nwo_safe_wtd, opened_nwo_safe_wtd, nwo_safe_wtd, -1, -1, -1, -1 },
> + { init_safe_wtd, -1, -1, -1, -1, -1, started_safe_wtd, -1 },
> + { nwo_safe_wtd, -1, -1, -1, -1, -1, started_nwo_safe_wtd, -1 },
> + { closed_running_safe_wtd, -1, -1, -1, -1, set_safe_wtd, -1, opened_safe_wtd },
> + { closed_running_safe_wtd, -1, -1, -1, safe_safe_wtd, -1, -1, stoped_safe_wtd },
> + { closed_running_nwo_safe_wtd, -1, -1, -1, safe_nwo_safe_wtd, -1, -1, -1 },
> + { -1, -1, -1, -1, safe_safe_wtd, -1, -1, -1 },
> + { -1, -1, -1, -1, safe_nwo_safe_wtd, -1, -1, -1 },
> + { closed_running_safe_wtd, -1, -1, -1, -1, set_safe_wtd, -1, stoped_safe_wtd },
> + { closed_running_nwo_safe_wtd, -1, -1, -1, -1, set_nwo_safe_wtd, -1, -1 },
> + { init_safe_wtd, -1, -1, -1, -1, -1, -1, -1 },
> + },
> + .initial_state = init_safe_wtd,
> + .final_states = { 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
I find this event table all but impossible to verify.
> +};
Powered by blists - more mailing lists