[PATCH 1/3] module: put modversions in vermagic

[PATCH 1/3] module: put modversions in vermagic

[Rusty Russell]

Don't allow a module built without versions altogether to be inserted
into a kernel which expects modversions.

modprobe --force will strip vermagic as well as modversions, so it
won't be effected, but this will make sure that a
non-CONFIG_MODVERSIONS module won't be accidentally inserted into a
CONFIG_MODVERSIONS kernel.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/vermagic.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff -r a39eaf516de9 include/linux/vermagic.h
--- a/include/linux/vermagic.h	Thu May 08 14:20:44 2008 +1000
+++ b/include/linux/vermagic.h	Thu May 08 15:18:47 2008 +1000
@@ -17,6 +17,11 @@
 #else
 #define MODULE_VERMAGIC_MODULE_UNLOAD ""
 #endif
+#ifdef CONFIG_MODVERSIONS
+#define MODULE_VERMAGIC_MODVERSIONS "modversions "
+#else
+#define MODULE_VERMAGIC_MODVERSIONS ""
+#endif
 #ifndef MODULE_ARCH_VERMAGIC
 #define MODULE_ARCH_VERMAGIC ""
 #endif
@@ -24,5 +29,6 @@
 #define VERMAGIC_STRING 						\
 	UTS_RELEASE " "							\
 	MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT 			\
-	MODULE_VERMAGIC_MODULE_UNLOAD MODULE_ARCH_VERMAGIC
+	MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS	\
+	MODULE_ARCH_VERMAGIC
 

[PATCH 2/3] module: be more picky about allowing missing module versions

[Rusty Russell]

We allow missing __versions sections, because modprobe --force strips
it.  It makes less sense to allow sections where there's no version
for a specific symbol the module uses, so disallow that.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 kernel/module.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff -r cf690cb1ffad kernel/module.c
--- a/kernel/module.c	Thu May 08 21:51:23 2008 +1000
+++ b/kernel/module.c	Thu May 08 21:51:48 2008 +1000
@@ -917,6 +917,10 @@ static int check_version(Elf_Shdr *sechd
 	if (!crc)
 		return 1;
 
+	/* No versions at all?  modprobe --force does this. */
+	if (versindex == 0)
+		return try_to_force_load(mod, symname) == 0;
+
 	versions = (void *) sechdrs[versindex].sh_addr;
 	num_versions = sechdrs[versindex].sh_size
 		/ sizeof(struct modversion_info);
@@ -932,8 +936,9 @@ static int check_version(Elf_Shdr *sechd
 		goto bad_version;
 	}
 
-	if (!try_to_force_load(mod, symname))
-		return 1;
+	printk(KERN_WARNING "%s: no symbol version for %s\n",
+	       mod->name, symname);
+	return 0;
 
 bad_version:
 	printk("%s: disagrees about version of symbol %s\n",

[PATCH 3/3] module: don't ignore vermagic string if module doesn't have modversions

[Rusty Russell]

Linus found a logic bug: we ignore the version number in a module's
vermagic string if we have CONFIG_MODVERSIONS set, but modversions
also lets through a module with no __versions section for modprobe
--force (with tainting, but still).

We should only ignore the start of the vermagic string if the module
actually *has* crcs to check.  Rather than (say) having an
entertaining hissy fit and creating a config option to work around the
buggy code.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 init/Kconfig    |    6 +++---
 kernel/module.c |   16 ++++++++++------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff -r 8805b380fdff init/Kconfig
--- a/init/Kconfig	Thu May 08 21:14:37 2008 +1000
+++ b/init/Kconfig	Thu May 08 21:16:20 2008 +1000
@@ -837,9 +837,9 @@ config MODULE_FORCE_LOAD
 	depends on MODULES
 	default n
 	help
-	  This option allows loading of modules even if that would set the
-          'F' (forced) taint, due to lack of version info.  Which is
-	  usually a really bad idea.
+	  Allow loading of modules without version information (ie. modprobe
+	  --force).  Forced module loading sets the 'F' (forced) taint flag and
+	  is usually a really bad idea.
 
 config MODULE_UNLOAD
 	bool "Module unloading"
diff -r 8805b380fdff kernel/module.c
--- a/kernel/module.c	Thu May 08 21:14:37 2008 +1000
+++ b/kernel/module.c	Thu May 08 21:16:20 2008 +1000
@@ -956,11 +956,14 @@ static inline int check_modstruct_versio
 	return check_version(sechdrs, versindex, "struct_module", mod, crc);
 }
 
-/* First part is kernel version, which we ignore. */
-static inline int same_magic(const char *amagic, const char *bmagic)
+/* First part is kernel version, which we ignore if module has crcs. */
+static inline int same_magic(const char *amagic, const char *bmagic,
+			     bool has_crcs)
 {
-	amagic += strcspn(amagic, " ");
-	bmagic += strcspn(bmagic, " ");
+	if (has_crcs) {
+		amagic += strcspn(amagic, " ");
+		bmagic += strcspn(bmagic, " ");
+	}
 	return strcmp(amagic, bmagic) == 0;
 }
 #else
@@ -980,7 +983,8 @@ static inline int check_modstruct_versio
 	return 1;
 }
 
-static inline int same_magic(const char *amagic, const char *bmagic)
+static inline int same_magic(const char *amagic, const char *bmagic,
+			     bool has_crcs)
 {
 	return strcmp(amagic, bmagic) == 0;
 }
@@ -1873,7 +1877,7 @@ static struct module *load_module(void _
 		err = try_to_force_load(mod, "magic");
 		if (err)
 			goto free_hdr;
-	} else if (!same_magic(modmagic, vermagic)) {
+	} else if (!same_magic(modmagic, vermagic, versindex)) {
 		printk(KERN_ERR "%s: version magic '%s' should be '%s'\n",
 		       mod->name, modmagic, vermagic);
 		err = -ENOEXEC;

Re: [PATCH 3/3] module: don't ignore vermagic string if module doesn't have modversions

[Linus Torvalds]

On Fri, 9 May 2008, Rusty Russell wrote:
> 
> We should only ignore the start of the vermagic string if the module
> actually *has* crcs to check.  Rather than (say) having an
> entertaining hissy fit and creating a config option to work around the
> buggy code.

Btw, this makes the two vesions of "same_magic()" be basically the same 
(since without MODVERSIONS, has_crcs *should* always be zero). Maybe we 
should make it just one version, to simplify logic?

		Linus

Re: [PATCH 3/3] module: don't ignore vermagic string if module doesn't have modversions

[Rusty Russell]

On Saturday 10 May 2008 00:44:34 Linus Torvalds wrote:
> On Fri, 9 May 2008, Rusty Russell wrote:
> > We should only ignore the start of the vermagic string if the module
> > actually *has* crcs to check.  Rather than (say) having an
> > entertaining hissy fit and creating a config option to work around the
> > buggy code.
>
> Btw, this makes the two vesions of "same_magic()" be basically the same
> (since without MODVERSIONS, has_crcs *should* always be zero). Maybe we
> should make it just one version, to simplify logic?

I know, but it's a little subtle.

If the module has an __versions section, yet the kernel was built without 
CONFIG_MODVERSIONS, we should not ignore the first part of the version.  We 
could just #ifdef CONFIG_MODVERSIONS around the verindex initialization tho, 
to guarantee that it's 0 for !MODVERSIONS.  But that puts YA #ifdef in that 
function, and I already itch to clean up the ones there...

Cheers,
Rusty.