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
