x86/kbuild: enable modversions for symbols exported from asm

Commit Message

Ben Hutchings Nov. 29, 2016, 7:57 p.m. UTC

On Tue, 2016-11-29 at 08:17 -0800, Linus Torvalds wrote:
> > On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <[e-mail]> wrote:
> > 
> > The original and easily observable bug is that were are not generating
> > symbol checksums for the asm-exported symbols, so they default to 0.
> > This can be seen e.g. in the Module.symvers file. This seemed like a
> > minor issue, because with the functions written in asm, the type
> > checking is rather weak (this has been the case even before Al's
> > patches). However, there is another bug that with _some_ toolchains /
> > architectures, the checksums do not default to 0, but they are simply
> > missing in the ___kcrctab* sections and the module loader complains. We
> > can of course research into the details of the second bug, but we
> > already know that we are not generating the checksums while we should be.
> 
> So let's just say that "toolchain is buggy" and make a missing kcrctab
> entry mean zero (or mean "matches anything"). And just shut up the
> warning.
> 
> I do *not* want to add random bandaids for something like a broken
> toolchain issue when I'd really rather just delete the feature.

If the modversion is missing then the fallback should be to a full
vermagic match, i.e. including the release string.  Something like
this (untested):


Ben.

Comments

Linus Torvalds Nov. 29, 2016, 8:35 p.m. UTC | #1

On Tue, Nov 29, 2016 at 11:57 AM, Ben Hutchings <[e-mail]> wrote:
>
> If the modversion is missing then the fallback should be to a full
> vermagic match, i.e. including the release string.  Something like
> this (untested):

This really seems way too complicated for this situation.

And it's wrong too. The whole point of modversions was that you didn't
want to do the full version check.

We already know there were *some* crc's (we checked that at the top of
check_version(), but we've also checked it in "same_magic()" - it's
what makes us ignore the exact version number), but this particular
symbol doesn't have a crc. Just let it through, because we have bugs
in binutils.

So your extra complexity logic seems actively wrong. It makes
MODVERSIONS not work at all, rather than limp along. You're better off
just not having MODVERSIONS.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/init/Kconfig b/init/Kconfig
index c4fbc1e55c25..34407f15e6d3 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1945,7 +1945,6 @@  config MODULE_FORCE_UNLOAD
 
 config MODVERSIONS
 	bool "Module versioning support"
-	depends on BROKEN
 	help
 	  Usually, you have to use modules compiled with your kernel.
 	  Saying Y here makes it sometimes possible to use modules
diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..78d61ae50bc5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -296,6 +296,12 @@  int unregister_module_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_module_notifier);
 
+enum {
+	MAGIC_NO_MATCH,
+	MAGIC_MATCH_NEED_CRC,
+	MAGIC_MATCH_EXACT
+};
+
 struct load_info {
 	Elf_Ehdr *hdr;
 	unsigned long len;
@@ -305,6 +311,7 @@  struct load_info {
 	struct _ddebug *debug;
 	unsigned int num_debug;
 	bool sig_ok;
+	int magic_match;
 #ifdef CONFIG_KALLSYMS
 	unsigned long mod_kallsyms_init_off;
 #endif
@@ -1268,13 +1275,14 @@  static unsigned long maybe_relocated(unsigned long crc,
 	return crc;
 }
 
-static int check_version(Elf_Shdr *sechdrs,
-			 unsigned int versindex,
+static int check_version(const struct load_info *info,
 			 const char *symname,
 			 struct module *mod,
 			 const unsigned long *crc,
 			 const struct module *crc_owner)
 {
+	Elf_Shdr *sechdrs = info->sechdrs;
+	unsigned int versindex = info->index.vers;
 	unsigned int i, num_versions;
 	struct modversion_info *versions;
 
@@ -1294,6 +1302,10 @@  static int check_version(Elf_Shdr *sechdrs,
 		if (strcmp(versions[i].name, symname) != 0)
 			continue;
 
+		/* Ignore dummy zero CRC */
+		if (versions[i].crc == 0)
+			break;
+
 		if (versions[i].crc == maybe_relocated(*crc, crc_owner))
 			return 1;
 		pr_debug("Found checksum %lX vs module %lX\n",
@@ -1301,6 +1313,9 @@  static int check_version(Elf_Shdr *sechdrs,
 		goto bad_version;
 	}
 
+	if (info->magic_match == MAGIC_MATCH_EXACT)
+		return 1;
+
 	pr_warn("%s: no symbol version for %s\n", mod->name, symname);
 	return 0;
 
@@ -1310,8 +1325,7 @@  static int check_version(Elf_Shdr *sechdrs,
 	return 0;
 }
 
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
-					  unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
 					  struct module *mod)
 {
 	const unsigned long *crc;
@@ -1327,24 +1341,24 @@  static inline int check_modstruct_version(Elf_Shdr *sechdrs,
 		BUG();
 	}
 	preempt_enable();
-	return check_version(sechdrs, versindex,
+	return check_version(info,
 			     VMLINUX_SYMBOL_STR(module_layout), mod, crc,
 			     NULL);
 }
 
-/* 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)
+/* First part is kernel version, which can be ignored if module has crcs. */
+static inline int compare_magic(const char *amagic, const char *bmagic)
 {
-	if (has_crcs) {
-		amagic += strcspn(amagic, " ");
-		bmagic += strcspn(bmagic, " ");
-	}
-	return strcmp(amagic, bmagic) == 0;
+	if (strcmp(amagic, bmagic) == 0)
+		return MAGIC_MATCH_EXACT;
+
+	amagic += strcspn(amagic, " ");
+	bmagic += strcspn(bmagic, " ");
+	return strcmp(amagic, bmagic) == 0 ? MAGIC_MATCH_NEED_CRC : MAGIC_NO_MATCH;
 }
+
 #else
-static inline int check_version(Elf_Shdr *sechdrs,
-				unsigned int versindex,
+static inline int check_version(const struct load_info *info,
 				const char *symname,
 				struct module *mod,
 				const unsigned long *crc,
@@ -1353,17 +1367,15 @@  static inline int check_version(Elf_Shdr *sechdrs,
 	return 1;
 }
 
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
-					  unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
 					  struct module *mod)
 {
 	return 1;
 }
 
-static inline int same_magic(const char *amagic, const char *bmagic,
-			     bool has_crcs)
+static inline int compare_magic(const char *amagic, const char *bmagic)
 {
-	return strcmp(amagic, bmagic) == 0;
+	return strcmp(amagic, bmagic) == 0 ? MAGIC_MATCH_EXACT : MAGIC_NO_MATCH;
 }
 #endif /* CONFIG_MODVERSIONS */
 
@@ -1390,8 +1402,7 @@  static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	if (!sym)
 		goto unlock;
 
-	if (!check_version(info->sechdrs, info->index.vers, name, mod, crc,
-			   owner)) {
+	if (!check_version(info, name, mod, crc, owner)) {
 		sym = ERR_PTR(-EINVAL);
 		goto getname;
 	}
@@ -2936,7 +2947,7 @@  static struct module *setup_load_info(struct load_info *info, int flags)
 	info->index.pcpu = find_pcpusec(info);
 
 	/* Check module struct version now, before we try to use module. */
-	if (!check_modstruct_version(info->sechdrs, info->index.vers, mod))
+	if (!check_modstruct_version(info, mod))
 		return ERR_PTR(-ENOEXEC);
 
 	return mod;
@@ -2952,13 +2963,19 @@  static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 
 	/* This is allowed: modprobe --force will invalidate it. */
 	if (!modmagic) {
+		info->magic_match = MAGIC_NO_MATCH;
 		err = try_to_force_load(mod, "bad vermagic");
 		if (err)
 			return err;
-	} else if (!same_magic(modmagic, vermagic, info->index.vers)) {
-		pr_err("%s: version magic '%s' should be '%s'\n",
-		       mod->name, modmagic, vermagic);
-		return -ENOEXEC;
+	} else {
+		info->magic_match = compare_magic(modmagic, vermagic);
+		if (info->magic_match == MAGIC_NO_MATCH ||
+		    (info->magic_match == MAGIC_MATCH_NEED_CRC &&
+		     !info->index.vers)) {
+			pr_err("%s: version magic '%s' should be '%s'\n",
+			       mod->name, modmagic, vermagic);
+			return -ENOEXEC;
+		}
 	}
 
 	if (!get_modinfo(info, "intree")) {
--- END ---