Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Ben Hutchings]

On Tue, 2016-11-29 at 08:17 -0800, Linus Torvalds wrote:
> > On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <mmarek@xxxxxxxx> 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):

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 ---

Ben.

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
                                - John Levine, moderator of comp.compilers

Attachment: signature.asc
Description: Digital signature