netdev - Re: [PATCH v2 01/15] taint: add module firmware crash taint support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200519164231.GA27392@linux-8ccs>
Date:   Tue, 19 May 2020 18:42:31 +0200
From:   Jessica Yu <jeyu@...nel.org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     akpm@...ux-foundation.org, arnd@...db.de, rostedt@...dmis.org,
        mingo@...hat.com, aquini@...hat.com, cai@....pw, dyoung@...hat.com,
        bhe@...hat.com, peterz@...radead.org, tglx@...utronix.de,
        gpiccoli@...onical.com, pmladek@...e.com, tiwai@...e.de,
        schlad@...e.de, andriy.shevchenko@...ux.intel.com,
        keescook@...omium.org, daniel.vetter@...ll.ch, will@...nel.org,
        mchehab+samsung@...nel.org, kvalo@...eaurora.org,
        davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/15] taint: add module firmware crash taint support

+++ Luis Chamberlain [15/05/20 21:28 +0000]:
>Device driver firmware can crash, and sometimes, this can leave your
>system in a state which makes the device or subsystem completely
>useless. Detecting this by inspecting /proc/sys/kernel/tainted instead
>of scraping some magical words from the kernel log, which is driver
>specific, is much easier. So instead provide a helper which lets drivers
>annotate this.
>
>Once this happens, scrapers can easily look for modules taint flags
>for a firmware crash. This will taint both the kernel and respective
>calling module.
>
>The new helper module_firmware_crashed() uses LOCKDEP_STILL_OK as this
>fact should in no way shape or form affect lockdep. This taint is device
>driver specific.
>
>Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>
>---
> Documentation/admin-guide/tainted-kernels.rst |  6 ++++++
> include/linux/kernel.h                        |  3 ++-
> include/linux/module.h                        | 13 +++++++++++++
> include/trace/events/module.h                 |  3 ++-
> kernel/module.c                               |  5 +++--
> kernel/panic.c                                |  1 +
> tools/debugging/kernel-chktaint               |  7 +++++++
> 7 files changed, 34 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
>index 71e9184a9079..92530f1d60ae 100644
>--- a/Documentation/admin-guide/tainted-kernels.rst
>+++ b/Documentation/admin-guide/tainted-kernels.rst
>@@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
>  15  _/K   32768  kernel has been live patched
>  16  _/X   65536  auxiliary taint, defined for and used by distros
>  17  _/T  131072  kernel was built with the struct randomization plugin
>+ 18  _/Q  262144  driver firmware crash annotation
> ===  ===  ======  ========================================================
>
> Note: The character ``_`` is representing a blank in this table to make reading
>@@ -162,3 +163,8 @@ More detailed explanation for tainting
>      produce extremely unusual kernel structure layouts (even performance
>      pathological ones), which is important to know when debugging. Set at
>      build time.
>+
>+ 18) ``Q`` used by device drivers to annotate that the device driver's firmware
>+     has crashed and the device's operation has been severely affected. The
>+     device may be left in a crippled state, requiring full driver removal /
>+     addition, system reboot, or it is unclear how long recovery will take.
>diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>index 04a5885cec1b..19e1541c82c7 100644
>--- a/include/linux/kernel.h
>+++ b/include/linux/kernel.h
>@@ -601,7 +601,8 @@ extern enum system_states {
> #define TAINT_LIVEPATCH			15
> #define TAINT_AUX			16
> #define TAINT_RANDSTRUCT		17
>-#define TAINT_FLAGS_COUNT		18
>+#define TAINT_FIRMWARE_CRASH		18
>+#define TAINT_FLAGS_COUNT		19
>
> struct taint_flag {
> 	char c_true;	/* character printed when tainted */
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 2c2e988bcf10..221200078180 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -697,6 +697,14 @@ static inline bool is_livepatch_module(struct module *mod)
> bool is_module_sig_enforced(void);
> void set_module_sig_enforced(void);
>
>+void add_taint_module(struct module *mod, unsigned flag,
>+		      enum lockdep_ok lockdep_ok);
>+
>+static inline void module_firmware_crashed(void)
>+{
>+	add_taint_module(THIS_MODULE, TAINT_FIRMWARE_CRASH, LOCKDEP_STILL_OK);
>+}

Just a nit: I think module_firmware_crashed() is a confusing name - it
doesn't really tell me what it's doing, and it's not really related to
the rest of the module_* symbols, which mostly have to do with module
loader/module specifics. Especially since a driver can be built-in, too.
How about taint_firmware_crashed() or something similar?

Also, I think we might crash in add_taint_module() if a driver is
built into the kernel, because THIS_MODULE will be null and there is
no null pointer check in add_taint_module(). We could unify the
CONFIG_MODULES and !CONFIG_MODULES stubs and either add an `if (mod)`
check in add_taint_module() or add an #ifdef MODULE check in the stub
itself to call add_taint() or add_taint_module() as appropriate. Hope
that makes sense.

Thanks!

Jessica