lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Aug 2020 10:54:02 +0900 From: Masahiro Yamada <masahiroy@...nel.org> To: Jessica Yu <jeyu@...nel.org> Cc: Ard Biesheuvel <ardb@...nel.org>, Will Deacon <will@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Szabolcs Nagy <szabolcs.nagy@....com>, Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>, Kees Cook <keescook@...omium.org>, Josh Poimboeuf <jpoimboe@...hat.com>, Miroslav Benes <mbenes@...e.cz>, Mark Rutland <mark.rutland@....com>, nd <nd@....com> Subject: Re: [PATCH v2] module: Harden STRICT_MODULE_RWX On Tue, Aug 25, 2020 at 12:24 AM Jessica Yu <jeyu@...nel.org> wrote: > > +++ Ard Biesheuvel [22/08/20 15:47 +0200]: > >(+ Masahiro) > > > >On Fri, 21 Aug 2020 at 14:30, Will Deacon <will@...nel.org> wrote: > >> > >> On Fri, Aug 21, 2020 at 02:27:05PM +0200, Ard Biesheuvel wrote: > >> > On Fri, 21 Aug 2020 at 14:20, Will Deacon <will@...nel.org> wrote: > >> > > > >> > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > >> > > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu <jeyu@...nel.org> wrote: > >> > > > > > >> > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > >> > > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra <peterz@...radead.org> wrote: > >> > > > > >> > >> > > > > >> On Wed, Aug 12, 2020 at 06:37:57PM +0200, Ard Biesheuvel wrote: > >> > > > > >> > I know there is little we can do at this point, apart from ignoring > >> > > > > >> > the permissions - perhaps we should just defer the w^x check until > >> > > > > >> > after calling module_frob_arch_sections()? > >> > > > > >> > >> > > > > >> My earlier suggestion was to ignore it for 0-sized sections. > >> > > > > > > >> > > > > >Only they are 1 byte sections in this case. > >> > > > > > > >> > > > > >We override the sh_type and sh_flags explicitly for these sections at > >> > > > > >module load time, so deferring the check seems like a reasonable > >> > > > > >alternative to me. > >> > > > > > >> > > > > So module_enforce_rwx_sections() is already called after > >> > > > > module_frob_arch_sections() - which really baffled me at first, since > >> > > > > sh_type and sh_flags should have been set already in > >> > > > > module_frob_arch_sections(). > >> > > > > > >> > > > > I added some debug prints to see which section the module code was > >> > > > > tripping on, and it was .text.ftrace_trampoline. See this snippet from > >> > > > > arm64's module_frob_arch_sections(): > >> > > > > > >> > > > > else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > >> > > > > !strcmp(secstrings + sechdrs[i].sh_name, > >> > > > > ".text.ftrace_trampoline")) > >> > > > > tramp = sechdrs + i; > >> > > > > > >> > > > > Since Mauro's config doesn't have CONFIG_DYNAMIC_FTRACE enabled, tramp > >> > > > > is never set here and the if (tramp) check at the end of the function > >> > > > > fails, so its section flags are never set, so they remain WAX and fail > >> > > > > the rwx check. > >> > > > > >> > > > Right. Our module.lds does not go through the preprocessor, so we > >> > > > cannot add the #ifdef check there currently. So we should either drop > >> > > > the IS_ENABLED() check here, or simply rename the section, dropping > >> > > > the .text prefix (which doesn't seem to have any significance outside > >> > > > this context) > >> > > > > >> > > > I'll leave it to Will to make the final call here. > >> > > > >> > > Why don't we just preprocess the linker script, like we do for the main > >> > > kernel? > >> > > > >> > > >> > That should work as well, I just haven't checked how straight-forward > >> > it is to change that. > >> > >> Ok, if it's _not_ straightforward, then let's just drop the IS_ENABLED() > >> altogether. > >> > > > >I played around with this for a while, but failed to get Kbuild to > >instantiate $(objtree)/arch/arm64/kernel/module.lds based on > >$(srctree)/arch/arm64/kernel/module.lds.S and the cpp_lds_S rule. > >Perhaps Masahiro has any suggestions here? Otherwise, let's just drop > >the IS_ENABLED() check for now. > > I also tinkered around a bit and was able to generate > $(objtree)/arch/arm64/kernel/module.lds based on > $(srctree)/arch/arm64/kernel/module.lds.S only if I specified the > former as the make target directly. Correct me if I'm wrong, but I > guess this might be because the single build targets would utilize > scripts/Makefile.build (where the cpp_lds_S rule is defined) while the > module-related Makefiles don't seem to support .lds.S -> .lds in > general.. Masahiro, how easy would it be to extend .lds.S -> .lds > support to module linker scripts as well? > > Thanks, > > Jessica If you want to generate $(objtree)/arch/arm64/kernel/module.lds, you need to tell it to the build system. The following seems to work, but please do NOT do this: ----------------->8-------------------- diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index b45f0124cc16..4ae398c21111 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -116,7 +116,7 @@ endif CHECKFLAGS += -D__aarch64__ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) -KBUILD_LDS_MODULE += $(srctree)/arch/arm64/kernel/module.lds +KBUILD_LDS_MODULE += arch/arm64/kernel/module.lds endif ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y) diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index a561cbb91d4d..693797e6db01 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -71,3 +71,5 @@ extra-y += $(head-y) vmlinux.lds ifeq ($(CONFIG_DEBUG_EFI),y) AFLAGS_head.o += -DVMLINUX_PATH="\"$(realpath $(objtree)/vmlinux)\"" endif + +always-$(CONFIG_MODULES) += module.lds diff --git a/arch/arm64/kernel/module.lds b/arch/arm64/kernel/module.lds.S similarity index 100% rename from arch/arm64/kernel/module.lds rename to arch/arm64/kernel/module.lds.S ----------------->8-------------------- The difference between arch/arm64/kernel/vmlinux.lds and arch/arm64/kernel/module.lds is that we must keep the latter for external module builds. 'make clean' cannot remove any build artifact used for external modules, but 'make mrproper' must remove them. Let me think about it. -- Best Regards Masahiro Yamada
Powered by blists - more mailing lists