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