| 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
| ||
|
Date: Tue, 13 Sep 2016 16:36:09 -0400
From: Jessica Yu <jeyu@...hat.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Rusty Russell <rusty@...tcorp.com.au>,
Andrew Morton <akpm@...ux-foundation.org>,
Jiri Kosina <jkosina@...e.cz>,
Josh Poimboeuf <jpoimboe@...hat.com>,
live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: taint/module: Clean up global and module taint flags handling
+++ Petr Mladek [12/09/16 16:13 +0200]:
>The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
>add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
>potentially print one more character. But it did not increase the
>size of the corresponding buffers in m_show() and print_modules().
>
>We have recently done the same mistake when adding a taint flag
>for livepatching, see
>https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoimboe@redhat.com
>
>Also struct module uses an incompatible type for mod-taints flags.
>It survived from the commit 2bc2d61a9638dab670d ("[PATCH] list module
>taint flags in Oops/panic"). There was used "int" for the global taint
>flags at these times. But only the global tain flags was later changed
>to "unsigned long" by the commit 25ddbb18aae33ad2 ("Make the taint
>flags reliable").
>
>This patch defines TAINT_FLAGS_COUNT that can be used to create
>arrays and buffers of the right size. Note that we could not use
>enum because the taint flag indexes are used also in assembly code.
>
>Then it reworks the table that describes the taint flags. The TAINT_*
>numbers can be used as the index. Instead, we add information
>if the taint flag is also shown per-module.
>
>Finally, it uses "unsigned long", bit operations, and the updated
>taint_flags table also for mod->taints.
>
>It is not optimal because only few taint flags can be printed by
>module_taint_flags(). But better be on the safe side. IMHO, it is
>not worth the optimization and this is a good compromise.
>
>Signed-off-by: Petr Mladek <pmladek@...e.com>
>---
>
>Changes against v1:
>
> + reverted the change to enums because it broke asm code
>
> + instead, forced the size of the taint_flags table;
> used taint numbers as the index; used the table also
> in module.c
>
> + fixed the type of mod->taints
>
>
> include/linux/kernel.h | 9 +++++++++
> include/linux/module.h | 2 +-
> kernel/module.c | 31 +++++++++++++----------------
> kernel/panic.c | 53 ++++++++++++++++++++++++--------------------------
> 4 files changed, 48 insertions(+), 47 deletions(-)
>
>diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>index d96a6118d26a..33e88ff3af40 100644
>--- a/include/linux/kernel.h
>+++ b/include/linux/kernel.h
>@@ -509,6 +509,15 @@ extern enum system_states {
> #define TAINT_UNSIGNED_MODULE 13
> #define TAINT_SOFTLOCKUP 14
> #define TAINT_LIVEPATCH 15
>+#define TAINT_FLAGS_COUNT 16
>+
>+struct taint_flag {
>+ char true; /* character printed when tained */
>+ char false; /* character printed when not tained */
s/tained/tainted
>+ bool module; /* also show as a per-module taint flag */
>+};
>+
>+extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
>
> extern const char hex_asc[];
> #define hex_asc_lo(x) hex_asc[((x) & 0x0f)]
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 0c3207d26ac0..f6ee569c62bb 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -399,7 +399,7 @@ struct module {
> /* Arch-specific module values */
> struct mod_arch_specific arch;
>
>- unsigned int taints; /* same bits as kernel:tainted */
>+ unsigned long taints; /* same bits as kernel:taint_flags */
>
> #ifdef CONFIG_GENERIC_BUG
> /* Support for BUG */
>diff --git a/kernel/module.c b/kernel/module.c
>index 529efae9f481..303e03e08b51 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -330,7 +330,7 @@ static inline void add_taint_module(struct module *mod, unsigned flag,
> enum lockdep_ok lockdep_ok)
> {
> add_taint(flag, lockdep_ok);
>- mod->taints |= (1U << flag);
>+ set_bit(flag, &mod->taints);
> }
>
> /*
>@@ -1138,22 +1138,13 @@ static inline int module_unload_init(struct module *mod)
> static size_t module_flags_taint(struct module *mod, char *buf)
> {
> size_t l = 0;
>+ int i;
>+
>+ for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
>+ if (taint_flags[i].module && test_bit(i, &mod->taints))
>+ buf[l++] = taint_flags[i].true;
>+ }
>
>- if (mod->taints & (1 << TAINT_PROPRIETARY_MODULE))
>- buf[l++] = 'P';
>- if (mod->taints & (1 << TAINT_OOT_MODULE))
>- buf[l++] = 'O';
>- if (mod->taints & (1 << TAINT_FORCED_MODULE))
>- buf[l++] = 'F';
>- if (mod->taints & (1 << TAINT_CRAP))
>- buf[l++] = 'C';
>- if (mod->taints & (1 << TAINT_UNSIGNED_MODULE))
>- buf[l++] = 'E';
>- /*
>- * TAINT_FORCED_RMMOD: could be added.
>- * TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't
>- * apply to modules.
>- */
> return l;
> }
>
>@@ -4036,6 +4027,10 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> }
> #endif /* CONFIG_KALLSYMS */
>
>+/* Maximum number of characters written by module_flags() */
>+#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
>+
>+/* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
> static char *module_flags(struct module *mod, char *buf)
> {
> int bx = 0;
>@@ -4080,7 +4075,7 @@ static void m_stop(struct seq_file *m, void *p)
> static int m_show(struct seq_file *m, void *p)
> {
> struct module *mod = list_entry(p, struct module, list);
>- char buf[8];
>+ char buf[MODULE_FLAGS_BUF_SIZE];
>
> /* We always ignore unformed modules. */
> if (mod->state == MODULE_STATE_UNFORMED)
>@@ -4251,7 +4246,7 @@ EXPORT_SYMBOL_GPL(__module_text_address);
> void print_modules(void)
> {
> struct module *mod;
>- char buf[8];
>+ char buf[MODULE_FLAGS_BUF_SIZE];
>
> printk(KERN_DEFAULT "Modules linked in:");
> /* Most callers should already have preempt disabled, but make sure */
>diff --git a/kernel/panic.c b/kernel/panic.c
>index ca8cea1ef673..36d4fa264b2c 100644
>--- a/kernel/panic.c
>+++ b/kernel/panic.c
>@@ -265,30 +265,27 @@ void panic(const char *fmt, ...)
>
> EXPORT_SYMBOL(panic);
>
>-
>-struct tnt {
>- u8 bit;
>- char true;
>- char false;
>-};
>-
>-static const struct tnt tnts[] = {
>- { TAINT_PROPRIETARY_MODULE, 'P', 'G' },
>- { TAINT_FORCED_MODULE, 'F', ' ' },
>- { TAINT_CPU_OUT_OF_SPEC, 'S', ' ' },
>- { TAINT_FORCED_RMMOD, 'R', ' ' },
>- { TAINT_MACHINE_CHECK, 'M', ' ' },
>- { TAINT_BAD_PAGE, 'B', ' ' },
>- { TAINT_USER, 'U', ' ' },
>- { TAINT_DIE, 'D', ' ' },
>- { TAINT_OVERRIDDEN_ACPI_TABLE, 'A', ' ' },
>- { TAINT_WARN, 'W', ' ' },
>- { TAINT_CRAP, 'C', ' ' },
>- { TAINT_FIRMWARE_WORKAROUND, 'I', ' ' },
>- { TAINT_OOT_MODULE, 'O', ' ' },
>- { TAINT_UNSIGNED_MODULE, 'E', ' ' },
>- { TAINT_SOFTLOCKUP, 'L', ' ' },
>- { TAINT_LIVEPATCH, 'K', ' ' },
>+/*
>+ * TAINT_FORCED_RMMOD could be a per-module flag but the module
>+ * is being removed anyway.
>+ */
>+const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
>+ { 'P', 'G', true }, /* TAINT_PROPRIETARY_MODULE */
>+ { 'F', ' ', true }, /* TAINT_FORCED_MODULE */
>+ { 'S', ' ', false }, /* TAINT_CPU_OUT_OF_SPEC */
>+ { 'R', ' ', false }, /* TAINT_FORCED_RMMOD */
>+ { 'M', ' ', false }, /* TAINT_MACHINE_CHECK */
>+ { 'B', ' ', false }, /* TAINT_BAD_PAGE */
>+ { 'U', ' ', false }, /* TAINT_USER */
>+ { 'D', ' ', false }, /* TAINT_DIE */
>+ { 'A', ' ', false }, /* TAINT_OVERRIDDEN_ACPI_TABLE */
>+ { 'W', ' ', false }, /* TAINT_WARN */
>+ { 'C', ' ', true }, /* TAINT_CRAP */
>+ { 'I', ' ', false }, /* TAINT_FIRMWARE_WORKAROUND */
>+ { 'O', ' ', true }, /* TAINT_OOT_MODULE */
>+ { 'E', ' ', true }, /* TAINT_UNSIGNED_MODULE */
>+ { 'L', ' ', false }, /* TAINT_SOFTLOCKUP */
>+ { 'K', ' ', false }, /* TAINT_LIVEPATCH */
This should be true here, right? TAINT_LIVEPATCH has been made a
module-specific taint by commit 2992ef29ae ("livepatch/module: make
TAINT_LIVEPATCH module-specific").
I think the rest looks fine, thanks for working on the cleanups.
Jessica
> };
>
> /**
>@@ -315,16 +312,16 @@ static const struct tnt tnts[] = {
> */
> const char *print_tainted(void)
> {
>- static char buf[ARRAY_SIZE(tnts) + sizeof("Tainted: ")];
>+ static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")];
>
> if (tainted_mask) {
> char *s;
> int i;
>
> s = buf + sprintf(buf, "Tainted: ");
>- for (i = 0; i < ARRAY_SIZE(tnts); i++) {
>- const struct tnt *t = &tnts[i];
>- *s++ = test_bit(t->bit, &tainted_mask) ?
>+ for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
>+ const struct taint_flag *t = &taint_flags[i];
>+ *s++ = test_bit(i, &tainted_mask) ?
> t->true : t->false;
> }
> *s = 0;
>--
>1.8.5.6
>
Powered by blists - more mailing lists