From patchwork Tue Nov 29 19:57:21 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Hutchings X-Patchwork-Id: 9453071 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id F17C26071E for ; Tue, 29 Nov 2016 19:57:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E4CC728365 for ; Tue, 29 Nov 2016 19:57:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D46DA283D3; Tue, 29 Nov 2016 19:57:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 058C428391 for ; Tue, 29 Nov 2016 19:57:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756713AbcK2T5h (ORCPT ); Tue, 29 Nov 2016 14:57:37 -0500 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:51587 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755083AbcK2T5g (ORCPT ); Tue, 29 Nov 2016 14:57:36 -0500 Received: from ben by shadbolt.decadent.org.uk with local (Exim 4.84_2) (envelope-from ) id 1cBoWn-0005LO-7c; Tue, 29 Nov 2016 19:57:23 +0000 Date: Tue, 29 Nov 2016 19:57:21 +0000 From: Ben Hutchings To: Linus Torvalds , Michal Marek Cc: Adam Borowski , Greg Kroah-Hartman , Linux Kbuild mailing list , Debian kernel maintainers , "linux-arch@vger.kernel.org" , Arnd Bergmann , Ingo Molnar , Nicholas Piggin , Linux Kernel Mailing List Message-ID: <20161129195721.GI2697@decadent.org.uk> References: <20161129131922.GA31466@angband.pl> <20161129135118.24696-1-kilobyte@angband.pl> <30bb2db4-47bd-0c35-8328-ef032b551f06@suse.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: X-SA-Exim-Mail-From: ben@decadent.org.uk Subject: Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on shadbolt.decadent.org.uk) Sender: linux-kbuild-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kbuild@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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): Ben. 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 ---