Re: [PATCH] list module taint flags in Oops/panic

[Florin Malita <fmalita@gmail.com>]

Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@xenotime.net>
>
> When listing loaded modules during an oops or panic, also list each
> module's Tainted flags if non-zero (P: Proprietary or F: Forced load only).
>   

Funny, I was playing with something very similar last weekend, then I
noticed your patch merged into mainline ;) So here are some comments...

> --- linux-2618-g10.orig/include/linux/module.h
> +++ linux-2618-g10/include/linux/module.h
> @@ -320,6 +320,8 @@ struct module
>  	/* Am I GPL-compatible */
>  	int license_gplok;
>  
> +	unsigned int taints;	/* same bits as kernel:tainted */
>   

No need to keep 'license_gplok' around anymore, it should be equivalent
to !(taints & TAINT_PROPRIETARY_MODULE).

> @@ -851,6 +851,7 @@ static int check_version(Elf_Shdr *sechd
>  		printk("%s: no version for \"%s\" found: kernel tainted.\n",
>  		       mod->name, symname);
>  		add_taint(TAINT_FORCED_MODULE);
> +		mod->taints |= TAINT_FORCED_MODULE;
>  	}
>   

This seems wrong, it only dirties mod->taints if the kernel is not
already F-tainted (the branch is conditioned by !(tainted &
TAINT_FORCED_MODULE)). So only the first forcefully-loaded module gets
its F bit set, which is probably not the intention...

> @@ -1325,6 +1326,7 @@ static void set_license(struct module *m
>  		printk(KERN_WARNING "%s: module license '%s' taints kernel.\n",
>  		       mod->name, license);
>  		add_taint(TAINT_PROPRIETARY_MODULE);
> +		mod->taints |= TAINT_PROPRIETARY_MODULE;
>  	}
>   

Similarly here, will only take the branch upon loading the first
proprietary module (conditional on !(tainted & TAINT_PROPRIETARY_MODULE)).

The currently merged version also has a problem in taint_flags():

static char *taint_flags(unsigned int taints, char *buf)
{
        *buf = '\0';
        if (taints) {
                int bx;

                buf[0] = '(';
                bx = 1;
                if (taints & TAINT_PROPRIETARY_MODULE)
                        buf[bx++] = 'P';
                if (taints & TAINT_FORCED_MODULE)
                        buf[bx++] = 'F';
                /*
                 * TAINT_FORCED_RMMOD: could be added.
                 * TAINT_UNSAFE_SMP, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE
don't
                 * apply to modules.
                 */
                buf[bx] = ')';
        }
        return buf;
}

The buffer is not NULL-terminated after printing the flags.

Also, it would be nice to show per-module taint info in /proc/modules
too. So how about the following (applies on top of the merged
version/current git, tested):

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

 include/linux/module.h |    3 -
 kernel/module.c        |   94 +++++++++++++++++++++++++------------------------
 2 files changed, 49 insertions(+), 48 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 4b2d809..d1d00ce 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -317,9 +317,6 @@ struct module
 	/* Am I unsafe to unload? */
 	int unsafe;
 
-	/* Am I GPL-compatible */
-	int license_gplok;
-
 	unsigned int taints;	/* same bits as kernel:tainted */
 
 #ifdef CONFIG_MODULE_UNLOAD
diff --git a/kernel/module.c b/kernel/module.c
index 7c77a0a..a258cd5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -87,6 +87,12 @@ static inline int strong_try_module_get(
 	return try_module_get(mod);
 }
 
+static inline void add_taint_module(struct module *mod, unsigned flag)
+{
+	add_taint(flag);
+	mod->taints |= flag;
+}
+
 /* A thread that wants to hold a reference to a module only while it
  * is running can call ths to safely exit.
  * nfsd and lockd use this.
@@ -847,12 +853,10 @@ static int check_version(Elf_Shdr *sechd
 		return 0;
 	}
 	/* Not in module's version table.  OK, but that taints the kernel. */
-	if (!(tainted & TAINT_FORCED_MODULE)) {
+	if (!(tainted & TAINT_FORCED_MODULE))
 		printk("%s: no version for \"%s\" found: kernel tainted.\n",
 		       mod->name, symname);
-		add_taint(TAINT_FORCED_MODULE);
-		mod->taints |= TAINT_FORCED_MODULE;
-	}
+	add_taint_module(mod, TAINT_FORCED_MODULE);
 	return 1;
 }
 
@@ -910,7 +914,8 @@ static unsigned long resolve_symbol(Elf_
 	unsigned long ret;
 	const unsigned long *crc;
 
-	ret = __find_symbol(name, &owner, &crc, mod->license_gplok);
+	ret = __find_symbol(name, &owner, &crc,
+			!(mod->taints & TAINT_PROPRIETARY_MODULE));
 	if (ret) {
 		/* use_module can fail due to OOM, or module unloading */
 		if (!check_version(sechdrs, versindex, name, mod, crc) ||
@@ -1335,12 +1340,11 @@ static void set_license(struct module *m
 	if (!license)
 		license = "unspecified";
 
-	mod->license_gplok = license_is_gpl_compatible(license);
-	if (!mod->license_gplok && !(tainted & TAINT_PROPRIETARY_MODULE)) {
-		printk(KERN_WARNING "%s: module license '%s' taints kernel.\n",
-		       mod->name, license);
-		add_taint(TAINT_PROPRIETARY_MODULE);
-		mod->taints |= TAINT_PROPRIETARY_MODULE;
+	if (!license_is_gpl_compatible(license)) {
+		if (!(tainted & TAINT_PROPRIETARY_MODULE))
+			printk(KERN_WARNING "%s: module license '%s' taints"
+				"kernel.\n", mod->name, license);
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE);
 	}
 }
 
@@ -1619,8 +1623,7 @@ #endif
 	modmagic = get_modinfo(sechdrs, infoindex, "vermagic");
 	/* This is allowed: modprobe --force will invalidate it. */
 	if (!modmagic) {
-		add_taint(TAINT_FORCED_MODULE);
-		mod->taints |= TAINT_FORCED_MODULE;
+		add_taint_module(mod, TAINT_FORCED_MODULE);
 		printk(KERN_WARNING "%s: no version magic, tainting kernel.\n",
 		       mod->name);
 	} else if (!same_magic(modmagic, vermagic)) {
@@ -1714,14 +1717,10 @@ #endif
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(sechdrs, infoindex, "license"));
 
-	if (strcmp(mod->name, "ndiswrapper") == 0) {
-		add_taint(TAINT_PROPRIETARY_MODULE);
-		mod->taints |= TAINT_PROPRIETARY_MODULE;
-	}
-	if (strcmp(mod->name, "driverloader") == 0) {
-		add_taint(TAINT_PROPRIETARY_MODULE);
-		mod->taints |= TAINT_PROPRIETARY_MODULE;
-	}
+	if (strcmp(mod->name, "ndiswrapper") == 0)
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE);
+	if (strcmp(mod->name, "driverloader") == 0)
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE);
 
 	/* Set up MODINFO_ATTR fields */
 	setup_modinfo(mod, sechdrs, infoindex);
@@ -1766,8 +1765,7 @@ #ifdef CONFIG_MODVERSIONS
 	    (mod->num_unused_gpl_syms && !unusedgplcrcindex)) {
 		printk(KERN_WARNING "%s: No versions for exported symbols."
 		       " Tainting kernel.\n", mod->name);
-		add_taint(TAINT_FORCED_MODULE);
-		mod->taints |= TAINT_FORCED_MODULE;
+		add_taint_module(mod, TAINT_FORCED_MODULE);
 	}
 #endif
 
@@ -2131,9 +2129,33 @@ static void m_stop(struct seq_file *m, v
 	mutex_unlock(&module_mutex);
 }
 
+static char *taint_flags(unsigned int taints, char *buf)
+{
+	int bx = 0;
+	
+	if (taints) {
+		buf[bx++] = '(';
+		if (taints & TAINT_PROPRIETARY_MODULE)
+			buf[bx++] = 'P';
+		if (taints & TAINT_FORCED_MODULE)
+			buf[bx++] = 'F';
+		/*
+		 * TAINT_FORCED_RMMOD: could be added.
+		 * TAINT_UNSAFE_SMP, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't
+		 * apply to modules.
+		 */
+		buf[bx++] = ')';
+	}
+	buf[bx] = '\0';
+
+	return buf;
+}
+
 static int m_show(struct seq_file *m, void *p)
 {
 	struct module *mod = list_entry(p, struct module, list);
+	char buf[8];
+
 	seq_printf(m, "%s %lu",
 		   mod->name, mod->init_size + mod->core_size);
 	print_unload_info(m, mod);
@@ -2146,6 +2168,10 @@ static int m_show(struct seq_file *m, vo
 	/* Used by oprofile and other similar tools. */
 	seq_printf(m, " 0x%p", mod->module_core);
 
+	/* Taints info */
+	if (mod->taints)
+		seq_printf(m, " %s", taint_flags(mod->taints, buf));
+
 	seq_printf(m, "\n");
 	return 0;
 }
@@ -2234,28 +2260,6 @@ struct module *module_text_address(unsig
 	return mod;
 }
 
-static char *taint_flags(unsigned int taints, char *buf)
-{
-	*buf = '\0';
-	if (taints) {
-		int bx;
-
-		buf[0] = '(';
-		bx = 1;
-		if (taints & TAINT_PROPRIETARY_MODULE)
-			buf[bx++] = 'P';
-		if (taints & TAINT_FORCED_MODULE)
-			buf[bx++] = 'F';
-		/*
-		 * TAINT_FORCED_RMMOD: could be added.
-		 * TAINT_UNSAFE_SMP, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't
-		 * apply to modules.
-		 */
-		buf[bx] = ')';
-	}
-	return buf;
-}
-
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)
 {