Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756747AbcK2T5p (ORCPT ); Tue, 29 Nov 2016 14:57:45 -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 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-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="6Mt39TZj+HFMr11E" 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-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8428 Lines: 259 --6Mt39TZj+HFMr11E Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, 2016-11-29 at 08:17 -0800, Linus Torvalds wrote: > > On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek wrote: > >=20 > > 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 b= e. >=20 > 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. >=20 > 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 =20 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); =20 +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; } =20 -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 =3D info->sechdrs; + unsigned int versindex =3D info->index.vers; unsigned int i, num_versions; struct modversion_info *versions; =20 @@ -1294,6 +1302,10 @@ static int check_version(Elf_Shdr *sechdrs, if (strcmp(versions[i].name, symname) !=3D 0) continue; =20 + /* Ignore dummy zero CRC */ + if (versions[i].crc =3D=3D 0) + break; + if (versions[i].crc =3D=3D 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; } =20 + if (info->magic_match =3D=3D MAGIC_MATCH_EXACT) + return 1; + pr_warn("%s: no symbol version for %s\n", mod->name, symname); return 0; =20 @@ -1310,8 +1325,7 @@ static int check_version(Elf_Shdr *sechdrs, return 0; } =20 -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); } =20 -/* 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 +=3D strcspn(amagic, " "); - bmagic +=3D strcspn(bmagic, " "); - } - return strcmp(amagic, bmagic) =3D=3D 0; + if (strcmp(amagic, bmagic) =3D=3D 0) + return MAGIC_MATCH_EXACT; + + amagic +=3D strcspn(amagic, " "); + bmagic +=3D strcspn(bmagic, " "); + return strcmp(amagic, bmagic) =3D=3D 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; } =20 -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; } =20 -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) =3D=3D 0; + return strcmp(amagic, bmagic) =3D=3D 0 ? MAGIC_MATCH_EXACT : MAGIC_NO_MAT= CH; } #endif /* CONFIG_MODVERSIONS */ =20 @@ -1390,8 +1402,7 @@ static const struct kernel_symbol *resolve_symbol(str= uct module *mod, if (!sym) goto unlock; =20 - if (!check_version(info->sechdrs, info->index.vers, name, mod, crc, - owner)) { + if (!check_version(info, name, mod, crc, owner)) { sym =3D ERR_PTR(-EINVAL); goto getname; } @@ -2936,7 +2947,7 @@ static struct module *setup_load_info(struct load_inf= o *info, int flags) info->index.pcpu =3D find_pcpusec(info); =20 /* 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); =20 return mod; @@ -2952,13 +2963,19 @@ static int check_modinfo(struct module *mod, struct= load_info *info, int flags) =20 /* This is allowed: modprobe --force will invalidate it. */ if (!modmagic) { + info->magic_match =3D MAGIC_NO_MATCH; err =3D 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 =3D compare_magic(modmagic, vermagic); + if (info->magic_match =3D=3D MAGIC_NO_MATCH || + (info->magic_match =3D=3D MAGIC_MATCH_NEED_CRC && + !info->index.vers)) { + pr_err("%s: version magic '%s' should be '%s'\n", + mod->name, modmagic, vermagic); + return -ENOEXEC; + } } =20 if (!get_modinfo(info, "intree")) { --- END --- Ben. --=20 Ben Hutchings Theory and practice are closer in theory than in practice. =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0- John Levine, moderator of comp.compilers --6Mt39TZj+HFMr11E Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIUAwUBWD3doOe/yOyVhhEJAQrJiw/4k111hMHMZxTv8gfkR0zX/1FtwBtuO+mr Sa2KiRlJeQsn3asb7HuvU8VTU7TfsL8TLrFae314gIz7zpMAvg1tbH5PHxqDxkvy fIS8zJtCNsDGqbkJYUpQfEEOZic7l//taxw4Pb32k9i9YKIEBZ7zixfZzGySemQZ EnnaXwv7grpM4nTrdp7lD/pT6qmFlvJPz5qc5qVHvHDa23uY28etiq2v9JMq0Jjl FrR2+REpyla4B1SU6kkBD6mWImYp6Nhc5ID0tTiZ04bpfWXHc2XRiESszh1KOYOS Py1+v2bJ7l7/hh5Hi6e5joTGjG6tEWGQTd+re248B4ydu+qKcnV6A+g/BwhoDWHj Ap1kAXHC9f8IOxEaQrFZ4+BQtH7m9mp6+Y0oKFuxfn4BcEDLYvYIxbuTLIVRDfMg lm1lJJq5+Jc3+MtcCjHp8ENTSfoBcvCkhVzn4iu0K1gzfJuwA0JQPytjqHH/4q2V ngdqYIXJL5074LUf+CioZg5GWuBhu502iJcwqw2Js98+VMcgASW1IZT3TPK+gxMk 8DFIB5Fd7dgXjJLCIxEJPS4e6AQY75p7Zczgv82xvj5kr7lQmOhgWbxBrrl2vSAQ gNUkHpo26GuXk7pZ+EUnitzW2Rwp5YYU9mTlaeSNf4+vwvKamghWcW8n/veD0UWE AC0RfqiQaA== =zqRc -----END PGP SIGNATURE----- --6Mt39TZj+HFMr11E--