[PATCH] module/taint: Automatically increase the buffer size for new taint flags

[PATCH] module/taint: Automatically increase the buffer size for new taint flags

[Petr Mladek]

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

Let's convert the taint flags into enum and handle the buffer size
almost automatically.

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@suse.com>
---
 include/linux/kernel.h | 44 ++++++++++++++++++++++++--------------------
 kernel/module.c        |  8 ++++++--
 kernel/panic.c         |  4 ++--
 3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d96a6118d26a..1809bc82b7a5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
 	if (panic_timeout == arch_default_timeout)
 		panic_timeout = timeout;
 }
-extern const char *print_tainted(void);
 enum lockdep_ok {
 	LOCKDEP_STILL_OK,
 	LOCKDEP_NOW_UNRELIABLE
 };
-extern void add_taint(unsigned flag, enum lockdep_ok);
-extern int test_taint(unsigned flag);
-extern unsigned long get_taint(void);
 extern int root_mountflags;
 
 extern bool early_boot_irqs_disabled;
@@ -493,22 +489,30 @@ extern enum system_states {
 	SYSTEM_RESTART,
 } system_state;
 
-#define TAINT_PROPRIETARY_MODULE	0
-#define TAINT_FORCED_MODULE		1
-#define TAINT_CPU_OUT_OF_SPEC		2
-#define TAINT_FORCED_RMMOD		3
-#define TAINT_MACHINE_CHECK		4
-#define TAINT_BAD_PAGE			5
-#define TAINT_USER			6
-#define TAINT_DIE			7
-#define TAINT_OVERRIDDEN_ACPI_TABLE	8
-#define TAINT_WARN			9
-#define TAINT_CRAP			10
-#define TAINT_FIRMWARE_WORKAROUND	11
-#define TAINT_OOT_MODULE		12
-#define TAINT_UNSIGNED_MODULE		13
-#define TAINT_SOFTLOCKUP		14
-#define TAINT_LIVEPATCH			15
+enum taint_flags {
+	TAINT_PROPRIETARY_MODULE,	/*  0 */
+	TAINT_FORCED_MODULE,		/*  1 */
+	TAINT_CPU_OUT_OF_SPEC,		/*  2 */
+	TAINT_FORCED_RMMOD,		/*  3 */
+	TAINT_MACHINE_CHECK,		/*  4 */
+	TAINT_BAD_PAGE,			/*  5 */
+	TAINT_USER,			/*  6 */
+	TAINT_DIE,			/*  7 */
+	TAINT_OVERRIDDEN_ACPI_TABLE,	/*  8 */
+	TAINT_WARN,			/*  9 */
+	TAINT_CRAP,			/* 10 */
+	TAINT_FIRMWARE_WORKAROUND,	/* 11 */
+	TAINT_OOT_MODULE,		/* 12 */
+	TAINT_UNSIGNED_MODULE,		/* 13 */
+	TAINT_SOFTLOCKUP,		/* 14 */
+	TAINT_LIVEPATCH,		/* 15 */
+	TAINT_FLAGS_COUNT		/* keep last! */
+};
+
+extern const char *print_tainted(void);
+extern void add_taint(enum taint_flags flag, enum lockdep_ok);
+extern int test_taint(enum taint_flags flag);
+extern unsigned long get_taint(void);
 
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
diff --git a/kernel/module.c b/kernel/module.c
index 529efae9f481..fb6c0d425b47 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4036,6 +4036,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 +4084,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 +4255,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..e90125bf9238 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -334,7 +334,7 @@ const char *print_tainted(void)
 	return buf;
 }
 
-int test_taint(unsigned flag)
+int test_taint(enum taint_flags flag)
 {
 	return test_bit(flag, &tainted_mask);
 }
@@ -353,7 +353,7 @@ unsigned long get_taint(void)
  * If something bad has gone wrong, you'll want @lockdebug_ok = false, but for
  * some notewortht-but-not-corrupting cases, it can be set to true.
  */
-void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
+void add_taint(enum taint_flags flag, enum lockdep_ok lockdep_ok)
 {
 	if (lockdep_ok == LOCKDEP_NOW_UNRELIABLE && __debug_locks_off())
 		pr_warn("Disabling lock debugging due to kernel taint\n");
-- 
1.8.5.6

Re: [PATCH] module/taint: Automatically increase the buffer size for new taint flags

[kbuild test robot]

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

Hi Petr,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.8-rc5 next-20160907]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Petr-Mladek/module-taint-Automatically-increase-the-buffer-size-for-new-taint-flags/20160907-212318
config: arm64-alldefconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   /tmp/ccfR2Wf9.s: Assembler messages:
>> /tmp/ccfR2Wf9.s:799: Error: invalid operands (*UND* and *ABS* sections) for `<<'
   /tmp/ccfR2Wf9.s:840: Error: invalid operands (*UND* and *ABS* sections) for `<<'
   /tmp/ccfR2Wf9.s:987: Error: invalid operands (*UND* and *ABS* sections) for `<<'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 8344 bytes --]

Re: [PATCH] module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell]

Petr Mladek <pmladek@suse.com> writes:
> 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().

I agree, nice work!

Minor nitpick: the winged ' /* 0 */' comments imply the values matter,
but they don't.  I'd skip that.

I've CC'd Jessica to add to her review pile :)

Thanks,
Rusty.

> 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
>
> Let's convert the taint flags into enum and handle the buffer size
> almost automatically.
>
> 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@suse.com>
> ---
>  include/linux/kernel.h | 44 ++++++++++++++++++++++++--------------------
>  kernel/module.c        |  8 ++++++--
>  kernel/panic.c         |  4 ++--
>  3 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d96a6118d26a..1809bc82b7a5 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
>  	if (panic_timeout == arch_default_timeout)
>  		panic_timeout = timeout;
>  }
> -extern const char *print_tainted(void);
>  enum lockdep_ok {
>  	LOCKDEP_STILL_OK,
>  	LOCKDEP_NOW_UNRELIABLE
>  };
> -extern void add_taint(unsigned flag, enum lockdep_ok);
> -extern int test_taint(unsigned flag);
> -extern unsigned long get_taint(void);
>  extern int root_mountflags;
>  
>  extern bool early_boot_irqs_disabled;
> @@ -493,22 +489,30 @@ extern enum system_states {
>  	SYSTEM_RESTART,
>  } system_state;
>  
> -#define TAINT_PROPRIETARY_MODULE	0
> -#define TAINT_FORCED_MODULE		1
> -#define TAINT_CPU_OUT_OF_SPEC		2
> -#define TAINT_FORCED_RMMOD		3
> -#define TAINT_MACHINE_CHECK		4
> -#define TAINT_BAD_PAGE			5
> -#define TAINT_USER			6
> -#define TAINT_DIE			7
> -#define TAINT_OVERRIDDEN_ACPI_TABLE	8
> -#define TAINT_WARN			9
> -#define TAINT_CRAP			10
> -#define TAINT_FIRMWARE_WORKAROUND	11
> -#define TAINT_OOT_MODULE		12
> -#define TAINT_UNSIGNED_MODULE		13
> -#define TAINT_SOFTLOCKUP		14
> -#define TAINT_LIVEPATCH			15
> +enum taint_flags {
> +	TAINT_PROPRIETARY_MODULE,	/*  0 */
> +	TAINT_FORCED_MODULE,		/*  1 */
> +	TAINT_CPU_OUT_OF_SPEC,		/*  2 */
> +	TAINT_FORCED_RMMOD,		/*  3 */
> +	TAINT_MACHINE_CHECK,		/*  4 */
> +	TAINT_BAD_PAGE,			/*  5 */
> +	TAINT_USER,			/*  6 */
> +	TAINT_DIE,			/*  7 */
> +	TAINT_OVERRIDDEN_ACPI_TABLE,	/*  8 */
> +	TAINT_WARN,			/*  9 */
> +	TAINT_CRAP,			/* 10 */
> +	TAINT_FIRMWARE_WORKAROUND,	/* 11 */
> +	TAINT_OOT_MODULE,		/* 12 */
> +	TAINT_UNSIGNED_MODULE,		/* 13 */
> +	TAINT_SOFTLOCKUP,		/* 14 */
> +	TAINT_LIVEPATCH,		/* 15 */
> +	TAINT_FLAGS_COUNT		/* keep last! */
> +};
> +
> +extern const char *print_tainted(void);
> +extern void add_taint(enum taint_flags flag, enum lockdep_ok);
> +extern int test_taint(enum taint_flags flag);
> +extern unsigned long get_taint(void);
>  
>  extern const char hex_asc[];
>  #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
> diff --git a/kernel/module.c b/kernel/module.c
> index 529efae9f481..fb6c0d425b47 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4036,6 +4036,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 +4084,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 +4255,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..e90125bf9238 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -334,7 +334,7 @@ const char *print_tainted(void)
>  	return buf;
>  }
>  
> -int test_taint(unsigned flag)
> +int test_taint(enum taint_flags flag)
>  {
>  	return test_bit(flag, &tainted_mask);
>  }
> @@ -353,7 +353,7 @@ unsigned long get_taint(void)
>   * If something bad has gone wrong, you'll want @lockdebug_ok = false, but for
>   * some notewortht-but-not-corrupting cases, it can be set to true.
>   */
> -void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
> +void add_taint(enum taint_flags flag, enum lockdep_ok lockdep_ok)
>  {
>  	if (lockdep_ok == LOCKDEP_NOW_UNRELIABLE && __debug_locks_off())
>  		pr_warn("Disabling lock debugging due to kernel taint\n");
> -- 
> 1.8.5.6

Re: module/taint: Automatically increase the buffer size for new taint flags

[Jessica Yu]

+++ Petr Mladek [07/09/16 15: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
>
>Let's convert the taint flags into enum and handle the buffer size
>almost automatically.
>
>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@suse.com>
>---
> include/linux/kernel.h | 44 ++++++++++++++++++++++++--------------------
> kernel/module.c        |  8 ++++++--
> kernel/panic.c         |  4 ++--
> 3 files changed, 32 insertions(+), 24 deletions(-)
>
>diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>index d96a6118d26a..1809bc82b7a5 100644
>--- a/include/linux/kernel.h
>+++ b/include/linux/kernel.h
>@@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
> 	if (panic_timeout == arch_default_timeout)
> 		panic_timeout = timeout;
> }
>-extern const char *print_tainted(void);
> enum lockdep_ok {
> 	LOCKDEP_STILL_OK,
> 	LOCKDEP_NOW_UNRELIABLE
> };
>-extern void add_taint(unsigned flag, enum lockdep_ok);
>-extern int test_taint(unsigned flag);
>-extern unsigned long get_taint(void);
> extern int root_mountflags;
>
> extern bool early_boot_irqs_disabled;
>@@ -493,22 +489,30 @@ extern enum system_states {
> 	SYSTEM_RESTART,
> } system_state;
>
>-#define TAINT_PROPRIETARY_MODULE	0
>-#define TAINT_FORCED_MODULE		1
>-#define TAINT_CPU_OUT_OF_SPEC		2
>-#define TAINT_FORCED_RMMOD		3
>-#define TAINT_MACHINE_CHECK		4
>-#define TAINT_BAD_PAGE			5
>-#define TAINT_USER			6
>-#define TAINT_DIE			7
>-#define TAINT_OVERRIDDEN_ACPI_TABLE	8
>-#define TAINT_WARN			9
>-#define TAINT_CRAP			10
>-#define TAINT_FIRMWARE_WORKAROUND	11
>-#define TAINT_OOT_MODULE		12
>-#define TAINT_UNSIGNED_MODULE		13
>-#define TAINT_SOFTLOCKUP		14
>-#define TAINT_LIVEPATCH			15
>+enum taint_flags {
>+	TAINT_PROPRIETARY_MODULE,	/*  0 */
>+	TAINT_FORCED_MODULE,		/*  1 */
>+	TAINT_CPU_OUT_OF_SPEC,		/*  2 */
>+	TAINT_FORCED_RMMOD,		/*  3 */
>+	TAINT_MACHINE_CHECK,		/*  4 */
>+	TAINT_BAD_PAGE,			/*  5 */
>+	TAINT_USER,			/*  6 */
>+	TAINT_DIE,			/*  7 */
>+	TAINT_OVERRIDDEN_ACPI_TABLE,	/*  8 */
>+	TAINT_WARN,			/*  9 */
>+	TAINT_CRAP,			/* 10 */
>+	TAINT_FIRMWARE_WORKAROUND,	/* 11 */
>+	TAINT_OOT_MODULE,		/* 12 */
>+	TAINT_UNSIGNED_MODULE,		/* 13 */
>+	TAINT_SOFTLOCKUP,		/* 14 */
>+	TAINT_LIVEPATCH,		/* 15 */
>+	TAINT_FLAGS_COUNT		/* keep last! */
>+};

I liked the enum idea because we got TAINT_FLAGS_COUNT for free :-)
however I think we need to switch back to the #defines because of the kbuild
error.

The "Error: invalid operands...for `<<'" messages are related to the
__WARN_TAINT() macro (arch/arm64/include/asm/bug.h) which emits some assembly
that relies on the taint values. We don't have access to the enum values
in the assembler so we start getting things like:

        .short ((1 << 0) | ((TAINT_WARN) << 8))

where TAINT_WARN should have already been preprocessed, and this is where that
invalid operand error is coming from.

Jessica

Re: module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell]

Jessica Yu <jeyu@redhat.com> writes:
> I liked the enum idea because we got TAINT_FLAGS_COUNT for free :-)
> however I think we need to switch back to the #defines because of the kbuild
> error.
>
> The "Error: invalid operands...for `<<'" messages are related to the
> __WARN_TAINT() macro (arch/arm64/include/asm/bug.h) which emits some assembly
> that relies on the taint values. We don't have access to the enum values
> in the assembler so we start getting things like:
>
>         .short ((1 << 0) | ((TAINT_WARN) << 8))
>
> where TAINT_WARN should have already been preprocessed, and this is where that
> invalid operand error is coming from.

Yech.  They could use asm-offsets hacks to generate the values, but
I think you're right.

But I want a single table for taint flags anyway.  Let's pull the
one out of panic.c, declare it [TAINT_FLAGS_COUNT] so gcc will warn
if someone adds one and it no longer fits, and use it in module.c.

(Also make it indexed by flag, rather than containing the flag in the
struct).

Thanks,
Rusty.