Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Masahiro Yamada [31/08/20 19:42 +0900]:
[snipped for brevity]
Sorry for the delay.
Please try the attached patch.
Hi Masahiro,
Thank you for the patch. Sorry for the delay, I just wanted to report back
after briefly testing your patch. It works great, at the moment I've only
tested with arm64.
I made the following change to arch/arm64/include/asm/module.lds.h:
diff --git a/arch/arm64/include/asm/module.lds.h
b/arch/arm64/include/asm/module.lds.h
index 691f15af788e..d8e786e5fcdb 100644
--- a/arch/arm64/include/asm/module.lds.h
+++ b/arch/arm64/include/asm/module.lds.h
@@ -2,6 +2,8 @@
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
.init.plt (NOLOAD) : { BYTE(0) }
+#ifdef CONFIG_DYNAMIC_FTRACE
.text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
+#endif
}
#endif
Since originally we wanted to include .text.ftrace_trampoline only
conditionally.
The resulting scripts/module.lds looks correct with CONFIG_DYNAMIC_FTRACE=y:
SECTIONS {
/DISCARD/ : {
*(.discard)
*(.discard.*)
}
__ksymtab 0 : { *(SORT(___ksymtab+*)) }
__ksymtab_gpl 0 : { *(SORT(___ksymtab_gpl+*)) }
__ksymtab_unused 0 : { *(SORT(___ksymtab_unused+*)) }
__ksymtab_unused_gpl 0 : { *(SORT(___ksymtab_unused_gpl+*)) }
__ksymtab_gpl_future 0 : { *(SORT(___ksymtab_gpl_future+*)) }
__kcrctab 0 : { *(SORT(___kcrctab+*)) }
__kcrctab_gpl 0 : { *(SORT(___kcrctab_gpl+*)) }
__kcrctab_unused 0 : { *(SORT(___kcrctab_unused+*)) }
__kcrctab_unused_gpl 0 : { *(SORT(___kcrctab_unused_gpl+*)) }
__kcrctab_gpl_future 0 : { *(SORT(___kcrctab_gpl_future+*)) }
.init_array 0 : ALIGN(8) { *(SORT(.init_array.*)) *(.init_array) }
__jump_table 0 : ALIGN(8) { KEEP(*(__jump_table)) }
}
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
.init.plt (NOLOAD) : { BYTE(0) }
.text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
}
And with CONFIG_DYNAMIC_FTRACE=n as well:
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
.init.plt (NOLOAD) : { BYTE(0) }
}
I will test on more arches in the next days but in the meantime you could add my
Tested-by: Jessica Yu
Thank you for working on this!
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Mon, Aug 31, 2020 at 11:46:51AM +0200, Jessica Yu wrote: > +++ Will Deacon [21/08/20 13:30 +0100]: > [snipped] > > > > > > 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. > > Unfortunately I've been getting more reports about this issue, so let's just > get the aforementioned workaround merged first. Does the following look OK? > > diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > index 0ce3a28e3347..2e224435c024 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -305,8 +305,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr > *sechdrs, >mod->arch.core.plt_shndx = i; >else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt")) >mod->arch.init.plt_shndx = i; > - else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > -!strcmp(secstrings + sechdrs[i].sh_name, > + else if (!strcmp(secstrings + sechdrs[i].sh_name, > ".text.ftrace_trampoline")) >tramp = sechdrs + i; >else if (sechdrs[i].sh_type == SHT_SYMTAB) > > If so I'll turn it into a formal patch and we can get that merged in the next > -rc. Acked-by: Will Deacon Will
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Mon, Aug 31, 2020 at 10:25 PM Ard Biesheuvel wrote: > > On Mon, 31 Aug 2020 at 13:43, Masahiro Yamada wrote: > > > > On Mon, Aug 31, 2020 at 6:47 PM Jessica Yu wrote: > > > > > > +++ Will Deacon [21/08/20 13:30 +0100]: > > > [snipped] > > > >> > > > 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. > > > > > > Unfortunately I've been getting more reports about this issue, so let's > > > just > > > get the aforementioned workaround merged first. Does the following look > > > OK? > > > > > > diff --git a/arch/arm64/kernel/module-plts.c > > > b/arch/arm64/kernel/module-plts.c > > > index 0ce3a28e3347..2e224435c024 100644 > > > --- a/arch/arm64/kernel/module-plts.c > > > +++ b/arch/arm64/kernel/module-plts.c > > > @@ -305,8 +305,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, > > > Elf_Shdr *sechdrs, > > > mod->arch.core.plt_shndx = i; > > > else if (!strcmp(secstrings + sechdrs[i].sh_name, > > > ".init.plt")) > > > mod->arch.init.plt_shndx = i; > > > - else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > > > -!strcmp(secstrings + sechdrs[i].sh_name, > > > + else if (!strcmp(secstrings + sechdrs[i].sh_name, > > > ".text.ftrace_trampoline")) > > > tramp = sechdrs + i; > > > else if (sechdrs[i].sh_type == SHT_SYMTAB) > > > > > > If so I'll turn it into a formal patch and we can get that merged in the > > > next -rc. > > > > > > Thanks, > > > > > > Jessica > > > > > > > > Sorry for the delay. > > > > Please try the attached patch. > > > > Thanks Masahiro, > > I'll leave it to the maintainers to make the final call, but this does > look rather intrusive to me, especially for an -rc. Sure, I am OK with putting this off. > Renaming > scripts/module-common.lds to scripts/module.lds means that the distros > may have to update their scripts to generate the linux-headers > packages etc, It depends on how distributions implement scripts in their packages. It would be annoying if the packages were broken every time a new script is added. So, I expect the whole of scripts/ is copied to the module development package. The in-kernel deb package is OK because files under scripts/ are collected by the 'find' command. scripts/package/builddeb:59: find $(find arch/$SRCARCH -name include -o -name scripts -type d) -type f scripts/package/builddeb:67: find arch/$SRCARCH/include Module.symvers include scripts -type f The rpm packages are also OK. But, downstream packages should be updated if scripts/module-common.lds is hard-coded. > so if we do this at all, we'd better do it between > releases. Sure. We can go with Jessica's one-liner fix. I will post mine as a separate patch with linux-arch ML CCed to get more attention. -- Best Regards Masahiro Yamada
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Ard Biesheuvel [31/08/20 16:25 +0300]: On Mon, 31 Aug 2020 at 13:43, Masahiro Yamada wrote: On Mon, Aug 31, 2020 at 6:47 PM Jessica Yu wrote: > > +++ Will Deacon [21/08/20 13:30 +0100]: > [snipped] > >> > > > 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. > > Unfortunately I've been getting more reports about this issue, so let's just > get the aforementioned workaround merged first. Does the following look OK? > > diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > index 0ce3a28e3347..2e224435c024 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -305,8 +305,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > mod->arch.core.plt_shndx = i; > else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt")) > mod->arch.init.plt_shndx = i; > - else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > -!strcmp(secstrings + sechdrs[i].sh_name, > + else if (!strcmp(secstrings + sechdrs[i].sh_name, > ".text.ftrace_trampoline")) > tramp = sechdrs + i; > else if (sechdrs[i].sh_type == SHT_SYMTAB) > > If so I'll turn it into a formal patch and we can get that merged in the next -rc. > > Thanks, > > Jessica Sorry for the delay. Please try the attached patch. Thanks Masahiro, Yes, thanks Masahiro for looking into this! And no worries about the delay. I will be able to test the patch tomorrow. I'll leave it to the maintainers to make the final call, but this does look rather intrusive to me, especially for an -rc. Renaming scripts/module-common.lds to scripts/module.lds means that the distros may have to update their scripts to generate the linux-headers packages etc, so if we do this at all, we'd better do it between releases. Yes, agreed - I was suggesting dropping the IS_ENABLED() check for the next -rc so that the bug reports about this module loading issue stop cropping up, and the "proper" fix of supporting module.lds.S -> .lds would be suitable for the next release instead. Jessica
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Mon, 31 Aug 2020 at 13:43, Masahiro Yamada wrote: > > On Mon, Aug 31, 2020 at 6:47 PM Jessica Yu wrote: > > > > +++ Will Deacon [21/08/20 13:30 +0100]: > > [snipped] > > >> > > > 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. > > > > Unfortunately I've been getting more reports about this issue, so let's just > > get the aforementioned workaround merged first. Does the following look OK? > > > > diff --git a/arch/arm64/kernel/module-plts.c > > b/arch/arm64/kernel/module-plts.c > > index 0ce3a28e3347..2e224435c024 100644 > > --- a/arch/arm64/kernel/module-plts.c > > +++ b/arch/arm64/kernel/module-plts.c > > @@ -305,8 +305,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr > > *sechdrs, > > mod->arch.core.plt_shndx = i; > > else if (!strcmp(secstrings + sechdrs[i].sh_name, > > ".init.plt")) > > mod->arch.init.plt_shndx = i; > > - else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > > -!strcmp(secstrings + sechdrs[i].sh_name, > > + else if (!strcmp(secstrings + sechdrs[i].sh_name, > > ".text.ftrace_trampoline")) > > tramp = sechdrs + i; > > else if (sechdrs[i].sh_type == SHT_SYMTAB) > > > > If so I'll turn it into a formal patch and we can get that merged in the > > next -rc. > > > > Thanks, > > > > Jessica > > > > Sorry for the delay. > > Please try the attached patch. > Thanks Masahiro, I'll leave it to the maintainers to make the final call, but this does look rather intrusive to me, especially for an -rc. Renaming scripts/module-common.lds to scripts/module.lds means that the distros may have to update their scripts to generate the linux-headers packages etc, so if we do this at all, we'd better do it between releases.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Mon, Aug 31, 2020 at 6:47 PM Jessica Yu wrote:
>
> +++ Will Deacon [21/08/20 13:30 +0100]:
> [snipped]
> >> > > > 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.
>
> Unfortunately I've been getting more reports about this issue, so let's just
> get the aforementioned workaround merged first. Does the following look OK?
>
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
> index 0ce3a28e3347..2e224435c024 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/kernel/module-plts.c
> @@ -305,8 +305,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
> *sechdrs,
> mod->arch.core.plt_shndx = i;
> else if (!strcmp(secstrings + sechdrs[i].sh_name,
> ".init.plt"))
> mod->arch.init.plt_shndx = i;
> - else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
> -!strcmp(secstrings + sechdrs[i].sh_name,
> + else if (!strcmp(secstrings + sechdrs[i].sh_name,
> ".text.ftrace_trampoline"))
> tramp = sechdrs + i;
> else if (sechdrs[i].sh_type == SHT_SYMTAB)
>
> If so I'll turn it into a formal patch and we can get that merged in the next
> -rc.
>
> Thanks,
>
> Jessica
Sorry for the delay.
Please try the attached patch.
--
Best Regards
Masahiro Yamada
From 72b13233e843f47c6958cf0645653dc4a727515d Mon Sep 17 00:00:00 2001
From: Masahiro Yamada
Date: Fri, 28 Aug 2020 16:37:33 +0900
Subject: [PATCH] kbuild: preprocess module linker script
There was a request to preprocess the module linker script like we do
for the vmlinux one (https://lkml.org/lkml/2020/8/21/512).
The difference between vmlinux.lds and module.lds is that the latter
is needed for external module builds, thus must be cleaned up by
'make mrproper' instead of 'make clean' (also, it must be created by
'make modules_prepare').
We cannot put it in arch/*/kernel/ because 'make clean' descneds into
it.
scripts/module.lds is a good place because 'make clean' keeps all the
build artifacts under scripts/.
You can add arch-specific sections in , which is
included from scripts/module.lds.S.
Signed-off-by: Masahiro Yamada
---
Makefile | 1 -
arch/arm/Makefile | 4
.../{kernel/module.lds => include/asm/module.lds.h}| 2 ++
arch/arm64/Makefile| 4
.../{kernel/module.lds => include/asm/module.lds.h}| 2 ++
arch/ia64/Makefile | 1 -
arch/ia64/{module.lds => include/asm/module.lds.h} | 0
arch/m68k/Makefile | 1 -
.../{kernel/module.lds => include/asm/module.lds.h}| 0
arch/powerpc/Makefile | 1 -
.../{kernel/module.lds => include/asm/module.lds.h}| 0
arch/riscv/Makefile| 3 ---
.../{kernel/module.lds => include/asm/module.lds.h}| 3 ++-
arch/um/include/asm/Kbuild | 1 +
include/asm-generic/Kbuild | 1 +
include/asm-generic/module.lds.h | 10
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Will Deacon [21/08/20 13:30 +0100]: [snipped] > > > 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. Unfortunately I've been getting more reports about this issue, so let's just get the aforementioned workaround merged first. Does the following look OK? diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c index 0ce3a28e3347..2e224435c024 100644 --- a/arch/arm64/kernel/module-plts.c +++ b/arch/arm64/kernel/module-plts.c @@ -305,8 +305,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, mod->arch.core.plt_shndx = i; else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt")) mod->arch.init.plt_shndx = i; - else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && -!strcmp(secstrings + sechdrs[i].sh_name, + else if (!strcmp(secstrings + sechdrs[i].sh_name, ".text.ftrace_trampoline")) tramp = sechdrs + i; else if (sechdrs[i].sh_type == SHT_SYMTAB) If so I'll turn it into a formal patch and we can get that merged in the next -rc. Thanks, Jessica
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Tue, Aug 25, 2020 at 12:24 AM Jessica Yu wrote: > > +++ Ard Biesheuvel [22/08/20 15:47 +0200]: > >(+ Masahiro) > > > >On Fri, 21 Aug 2020 at 14:30, Will Deacon wrote: > >> > >> On Fri, Aug 21, 2020 at 02:27:05PM +0200, Ard Biesheuvel wrote: > >> > On Fri, 21 Aug 2020 at 14:20, Will Deacon wrote: > >> > > > >> > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > >> > > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > >> > > > > > >> > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > >> > > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra > >> > > > > > 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..4ae398c2 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
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Ard Biesheuvel [22/08/20 15:47 +0200]: (+ Masahiro) On Fri, 21 Aug 2020 at 14:30, Will Deacon wrote: On Fri, Aug 21, 2020 at 02:27:05PM +0200, Ard Biesheuvel wrote: > On Fri, 21 Aug 2020 at 14:20, Will Deacon wrote: > > > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > > > > > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra 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
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
(+ Masahiro) On Fri, 21 Aug 2020 at 14:30, Will Deacon wrote: > > On Fri, Aug 21, 2020 at 02:27:05PM +0200, Ard Biesheuvel wrote: > > On Fri, 21 Aug 2020 at 14:20, Will Deacon wrote: > > > > > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > > > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > > > > > > > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > > > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra > > > > > >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.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Fri, Aug 21, 2020 at 02:27:05PM +0200, Ard Biesheuvel wrote: > On Fri, 21 Aug 2020 at 14:20, Will Deacon wrote: > > > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > > > > > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra > > > > >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. Will
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Fri, 21 Aug 2020 at 14:20, Will Deacon wrote: > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > > > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra > > > >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.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote: > On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra 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? Will
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Thu, 13 Aug 2020 at 15:04, Jessica Yu wrote: > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]: > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra 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.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Ard Biesheuvel [13/08/20 10:36 +0200]: On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra 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.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Wed, Aug 12, 2020 at 05:42:05PM +0100, Szabolcs Nagy wrote: > The 08/12/2020 18:37, Ard Biesheuvel wrote: > > On Wed, 12 Aug 2020 at 18:00, Jessica Yu wrote: > > > +++ Szabolcs Nagy [12/08/20 15:15 +0100]: > > > >for me it bisects to > > > > > > > >https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8c803a2dd7d3d742a3d0071914f557ef465afe71 > > > > > > > >i will have to investigate further what's going on. > > > > > > Thanks for the hint. I'm almost certain it's due to this excerpt from > > > the changelog: "we now init sh_type and sh_flags for all known ABI > > > sections > > > in _bfd_elf_new_section_hook." > > > > > > Indeed, .plt and .text.* are listed as special sections in bfd/elf.c. > > > The former requires an exact match and the latter only has to match > > > the prefix ".text." Since the code considers ".plt" and > > > ".text.ftrace_trampoline" special sections, their sh_type and sh_flags > > > are now set by default. Now I guess the question is whether this can > > > be overriden by a linker script.. > > > > > > > If this is even possible to begin with, doing this in a way that is > > portable across the various linkers that we claim to support is going > > to be tricky. > > > > I suppose this is the downside of using partially linked objects as > > our module format - using ordinary shared libraries (along with the > > appropriate dynamic relocations which are mostly portable across > > architectures) would get rid of most of the PLT and trampoline issues, > > and of a lot of complex static relocation processing code. > > > > 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()? > > i opened > > https://sourceware.org/bugzilla/show_bug.cgi?id=26378 > > and waiting for binutils maintainers if this is intentional. Thanks, Szabolcs. It's looking this is intentional behaviour (a bug fix), so we'll have to suck it up in the module loader. Will
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra 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.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
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.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
The 08/12/2020 18:37, Ard Biesheuvel wrote:
> module_frob_arch_sections
>
> On Wed, 12 Aug 2020 at 18:00, Jessica Yu wrote:
> >
> > +++ Szabolcs Nagy [12/08/20 15:15 +0100]:
> > >The 08/12/2020 13:56, Will Deacon wrote:
> > >> On Wed, Aug 12, 2020 at 12:40:05PM +0200, pet...@infradead.org wrote:
> > >> > On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
> > >> > > The module .lds has BYTE(0) in the section contents to prevent the
> > >> > > linker from pruning them entirely. The (NOLOAD) is there to ensure
> > >> > > that this byte does not end up in the .ko, which is more a matter of
> > >> > > principle than anything else, so we can happily drop that if it
> > >> > > helps.
> > >> > >
> > >> > > However, this should only affect the PROGBITS vs NOBITS designation,
> > >> > > and so I am not sure whether it makes a difference.
> > >> > >
> > >> > > Depending on where the w^x check occurs, we might simply override the
> > >> > > permissions of these sections, and strip the writable permission if
> > >> > > it
> > >> > > is set in the PLT handling init code, which manipulates the metadata
> > >> > > of all these 3 sections before the module space is vmalloc'ed.
> > >> >
> > >> > What's curious is that this seems the result of some recent binutils
> > >> > change. Every build with binutils-2.34 (or older) does not seem to
> > >> > generate these as WAX, but has the much more sensible WA.
> > >> >
> > >> > I suppose we can change the kernel check and 'allow' W^X for 0 sized
> > >> > sections, but I think we should still figure out why binutils-2.35 is
> > >> > now generating WAX sections all of a sudden, it might come bite us
> > >> > elsewhere.
> > >>
> > >> Agreed, I think it's important to figure out what's going on here before
> > >> we
> > >> try to bodge around it.
> > >>
> > >> Adding Szabolcs, in case he has any ideas.
> > >>
> > >> To save him reading the whole thread, here's a summary:
> > >>
> > >> AArch64 kernel modules built with binutils 2.35 end up with a couple of
> > >> ELF sections marked as SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR:
> > >>
> > >> [ 5] .plt PROGBITS 0388 01d000 08 00 WAX 0 0 1
> > >> [ 6] .init.plt NOBITS 0390 01d008 08 00 WA 0 0 1
> > >> [ 7] .text.ftrace_trampoline PROGBITS 0398 01d008 08 00
> > >> WAX 0 0 1
> > >>
> > >> This results in the module being rejected by our loader, because we don't
> > >> permit writable, executable mappings.
> > >>
> > >> Our linker script for these entries uses NOLOAD, so it's odd to see
> > >> PROGBITS
> > >> appearing in the readelf output above (and older binutils emits NOBITS
> > >> sections). Anyway, here's the linker script:
> > >>
> > >> SECTIONS {
> > >> .plt (NOLOAD) : { BYTE(0) }
> > >> .init.plt (NOLOAD) : { BYTE(0) }
> > >> .text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
> > >> }
> > >>
> > >> It appears that the name of the section influences the behaviour, as
> > >> Jessica observed [1] that sections named .text.* end up with PROGBITS,
> > >> whereas random naming such as ".test" ends up with NOBITS, as before.
> > >>
> > >> We've looked at the changelog between binutils 2.34 and 2.35, but nothing
> > >> stands out. Any clues? Is this intentional binutils behaviour?
> > >
> > >for me it bisects to
> > >
> > >https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8c803a2dd7d3d742a3d0071914f557ef465afe71
> > >
> > >i will have to investigate further what's going on.
> >
> > Thanks for the hint. I'm almost certain it's due to this excerpt from
> > the changelog: "we now init sh_type and sh_flags for all known ABI sections
> > in _bfd_elf_new_section_hook."
> >
> > Indeed, .plt and .text.* are listed as special sections in bfd/elf.c.
> > The former requires an exact match and the latter only has to match
> > the prefix ".text." Since the code considers ".plt" and
> > ".text.ftrace_trampoline" special sections, their sh_type and sh_flags
> > are now set by default. Now I guess the question is whether this can
> > be overriden by a linker script..
> >
>
> If this is even possible to begin with, doing this in a way that is
> portable across the various linkers that we claim to support is going
> to be tricky.
>
> I suppose this is the downside of using partially linked objects as
> our module format - using ordinary shared libraries (along with the
> appropriate dynamic relocations which are mostly portable across
> architectures) would get rid of most of the PLT and trampoline issues,
> and of a lot of complex static relocation processing code.
>
> 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()?
i opened
https://sourceware.org/bugzilla/show_bug.cgi?id=26378
and waiting for binutils maintainers if this is intentional.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
module_frob_arch_sections
On Wed, 12 Aug 2020 at 18:00, Jessica Yu wrote:
>
> +++ Szabolcs Nagy [12/08/20 15:15 +0100]:
> >The 08/12/2020 13:56, Will Deacon wrote:
> >> On Wed, Aug 12, 2020 at 12:40:05PM +0200, pet...@infradead.org wrote:
> >> > On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
> >> > > The module .lds has BYTE(0) in the section contents to prevent the
> >> > > linker from pruning them entirely. The (NOLOAD) is there to ensure
> >> > > that this byte does not end up in the .ko, which is more a matter of
> >> > > principle than anything else, so we can happily drop that if it helps.
> >> > >
> >> > > However, this should only affect the PROGBITS vs NOBITS designation,
> >> > > and so I am not sure whether it makes a difference.
> >> > >
> >> > > Depending on where the w^x check occurs, we might simply override the
> >> > > permissions of these sections, and strip the writable permission if it
> >> > > is set in the PLT handling init code, which manipulates the metadata
> >> > > of all these 3 sections before the module space is vmalloc'ed.
> >> >
> >> > What's curious is that this seems the result of some recent binutils
> >> > change. Every build with binutils-2.34 (or older) does not seem to
> >> > generate these as WAX, but has the much more sensible WA.
> >> >
> >> > I suppose we can change the kernel check and 'allow' W^X for 0 sized
> >> > sections, but I think we should still figure out why binutils-2.35 is
> >> > now generating WAX sections all of a sudden, it might come bite us
> >> > elsewhere.
> >>
> >> Agreed, I think it's important to figure out what's going on here before we
> >> try to bodge around it.
> >>
> >> Adding Szabolcs, in case he has any ideas.
> >>
> >> To save him reading the whole thread, here's a summary:
> >>
> >> AArch64 kernel modules built with binutils 2.35 end up with a couple of
> >> ELF sections marked as SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR:
> >>
> >> [ 5] .plt PROGBITS 0388 01d000 08 00 WAX 0 0 1
> >> [ 6] .init.plt NOBITS 0390 01d008 08 00 WA 0 0 1
> >> [ 7] .text.ftrace_trampoline PROGBITS 0398 01d008 08 00
> >> WAX 0 0 1
> >>
> >> This results in the module being rejected by our loader, because we don't
> >> permit writable, executable mappings.
> >>
> >> Our linker script for these entries uses NOLOAD, so it's odd to see
> >> PROGBITS
> >> appearing in the readelf output above (and older binutils emits NOBITS
> >> sections). Anyway, here's the linker script:
> >>
> >> SECTIONS {
> >> .plt (NOLOAD) : { BYTE(0) }
> >> .init.plt (NOLOAD) : { BYTE(0) }
> >> .text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
> >> }
> >>
> >> It appears that the name of the section influences the behaviour, as
> >> Jessica observed [1] that sections named .text.* end up with PROGBITS,
> >> whereas random naming such as ".test" ends up with NOBITS, as before.
> >>
> >> We've looked at the changelog between binutils 2.34 and 2.35, but nothing
> >> stands out. Any clues? Is this intentional binutils behaviour?
> >
> >for me it bisects to
> >
> >https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8c803a2dd7d3d742a3d0071914f557ef465afe71
> >
> >i will have to investigate further what's going on.
>
> Thanks for the hint. I'm almost certain it's due to this excerpt from
> the changelog: "we now init sh_type and sh_flags for all known ABI sections
> in _bfd_elf_new_section_hook."
>
> Indeed, .plt and .text.* are listed as special sections in bfd/elf.c.
> The former requires an exact match and the latter only has to match
> the prefix ".text." Since the code considers ".plt" and
> ".text.ftrace_trampoline" special sections, their sh_type and sh_flags
> are now set by default. Now I guess the question is whether this can
> be overriden by a linker script..
>
If this is even possible to begin with, doing this in a way that is
portable across the various linkers that we claim to support is going
to be tricky.
I suppose this is the downside of using partially linked objects as
our module format - using ordinary shared libraries (along with the
appropriate dynamic relocations which are mostly portable across
architectures) would get rid of most of the PLT and trampoline issues,
and of a lot of complex static relocation processing code.
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()?
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Szabolcs Nagy [12/08/20 15:15 +0100]:
The 08/12/2020 13:56, Will Deacon wrote:
On Wed, Aug 12, 2020 at 12:40:05PM +0200, pet...@infradead.org wrote:
> On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
> > The module .lds has BYTE(0) in the section contents to prevent the
> > linker from pruning them entirely. The (NOLOAD) is there to ensure
> > that this byte does not end up in the .ko, which is more a matter of
> > principle than anything else, so we can happily drop that if it helps.
> >
> > However, this should only affect the PROGBITS vs NOBITS designation,
> > and so I am not sure whether it makes a difference.
> >
> > Depending on where the w^x check occurs, we might simply override the
> > permissions of these sections, and strip the writable permission if it
> > is set in the PLT handling init code, which manipulates the metadata
> > of all these 3 sections before the module space is vmalloc'ed.
>
> What's curious is that this seems the result of some recent binutils
> change. Every build with binutils-2.34 (or older) does not seem to
> generate these as WAX, but has the much more sensible WA.
>
> I suppose we can change the kernel check and 'allow' W^X for 0 sized
> sections, but I think we should still figure out why binutils-2.35 is
> now generating WAX sections all of a sudden, it might come bite us
> elsewhere.
Agreed, I think it's important to figure out what's going on here before we
try to bodge around it.
Adding Szabolcs, in case he has any ideas.
To save him reading the whole thread, here's a summary:
AArch64 kernel modules built with binutils 2.35 end up with a couple of
ELF sections marked as SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR:
[ 5] .plt PROGBITS 0388 01d000 08 00 WAX 0 0 1
[ 6] .init.plt NOBITS 0390 01d008 08 00 WA 0 0 1
[ 7] .text.ftrace_trampoline PROGBITS 0398 01d008 08 00 WAX 0
0 1
This results in the module being rejected by our loader, because we don't
permit writable, executable mappings.
Our linker script for these entries uses NOLOAD, so it's odd to see PROGBITS
appearing in the readelf output above (and older binutils emits NOBITS
sections). Anyway, here's the linker script:
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
.init.plt (NOLOAD) : { BYTE(0) }
.text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
}
It appears that the name of the section influences the behaviour, as
Jessica observed [1] that sections named .text.* end up with PROGBITS,
whereas random naming such as ".test" ends up with NOBITS, as before.
We've looked at the changelog between binutils 2.34 and 2.35, but nothing
stands out. Any clues? Is this intentional binutils behaviour?
for me it bisects to
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8c803a2dd7d3d742a3d0071914f557ef465afe71
i will have to investigate further what's going on.
Thanks for the hint. I'm almost certain it's due to this excerpt from
the changelog: "we now init sh_type and sh_flags for all known ABI sections
in _bfd_elf_new_section_hook."
Indeed, .plt and .text.* are listed as special sections in bfd/elf.c.
The former requires an exact match and the latter only has to match
the prefix ".text." Since the code considers ".plt" and
".text.ftrace_trampoline" special sections, their sh_type and sh_flags
are now set by default. Now I guess the question is whether this can
be overriden by a linker script..
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
The 08/12/2020 13:56, Will Deacon wrote:
> On Wed, Aug 12, 2020 at 12:40:05PM +0200, pet...@infradead.org wrote:
> > On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
> > > The module .lds has BYTE(0) in the section contents to prevent the
> > > linker from pruning them entirely. The (NOLOAD) is there to ensure
> > > that this byte does not end up in the .ko, which is more a matter of
> > > principle than anything else, so we can happily drop that if it helps.
> > >
> > > However, this should only affect the PROGBITS vs NOBITS designation,
> > > and so I am not sure whether it makes a difference.
> > >
> > > Depending on where the w^x check occurs, we might simply override the
> > > permissions of these sections, and strip the writable permission if it
> > > is set in the PLT handling init code, which manipulates the metadata
> > > of all these 3 sections before the module space is vmalloc'ed.
> >
> > What's curious is that this seems the result of some recent binutils
> > change. Every build with binutils-2.34 (or older) does not seem to
> > generate these as WAX, but has the much more sensible WA.
> >
> > I suppose we can change the kernel check and 'allow' W^X for 0 sized
> > sections, but I think we should still figure out why binutils-2.35 is
> > now generating WAX sections all of a sudden, it might come bite us
> > elsewhere.
>
> Agreed, I think it's important to figure out what's going on here before we
> try to bodge around it.
>
> Adding Szabolcs, in case he has any ideas.
>
> To save him reading the whole thread, here's a summary:
>
> AArch64 kernel modules built with binutils 2.35 end up with a couple of
> ELF sections marked as SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR:
>
> [ 5] .plt PROGBITS 0388 01d000 08 00 WAX 0 0 1
> [ 6] .init.plt NOBITS 0390 01d008 08 00 WA 0 0 1
> [ 7] .text.ftrace_trampoline PROGBITS 0398 01d008 08 00 WAX
> 0 0 1
>
> This results in the module being rejected by our loader, because we don't
> permit writable, executable mappings.
>
> Our linker script for these entries uses NOLOAD, so it's odd to see PROGBITS
> appearing in the readelf output above (and older binutils emits NOBITS
> sections). Anyway, here's the linker script:
>
> SECTIONS {
> .plt (NOLOAD) : { BYTE(0) }
> .init.plt (NOLOAD) : { BYTE(0) }
> .text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
> }
>
> It appears that the name of the section influences the behaviour, as
> Jessica observed [1] that sections named .text.* end up with PROGBITS,
> whereas random naming such as ".test" ends up with NOBITS, as before.
>
> We've looked at the changelog between binutils 2.34 and 2.35, but nothing
> stands out. Any clues? Is this intentional binutils behaviour?
for me it bisects to
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8c803a2dd7d3d742a3d0071914f557ef465afe71
i will have to investigate further what's going on.
>
> Thanks,
>
> Will
>
> [1] https://lore.kernel.org/r/20200812114127.ga10...@linux-8ccs.fritz.box
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Wed, Aug 12, 2020 at 4:42 AM Jessica Yu via Binutils
wrote:
>
> +++ pet...@infradead.org [12/08/20 12:40 +0200]:
> >On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
> >> The module .lds has BYTE(0) in the section contents to prevent the
> >> linker from pruning them entirely. The (NOLOAD) is there to ensure
> >> that this byte does not end up in the .ko, which is more a matter of
> >> principle than anything else, so we can happily drop that if it helps.
> >>
> >> However, this should only affect the PROGBITS vs NOBITS designation,
> >> and so I am not sure whether it makes a difference.
> >>
> >> Depending on where the w^x check occurs, we might simply override the
> >> permissions of these sections, and strip the writable permission if it
> >> is set in the PLT handling init code, which manipulates the metadata
> >> of all these 3 sections before the module space is vmalloc'ed.
> >
> >What's curious is that this seems the result of some recent binutils
> >change. Every build with binutils-2.34 (or older) does not seem to
> >generate these as WAX, but has the much more sensible WA.
> >
> >I suppose we can change the kernel check and 'allow' W^X for 0 sized
> >sections, but I think we should still figure out why binutils-2.35 is
> >now generating WAX sections all of a sudden, it might come bite us
> >elsewhere.
>
> I have just tested with binutils-2.35 and am observing the same
> behavior. Both .plt and .text.ftrace_trampoline end up with
> SHT_PROGBITS and are marked 'WAX'. With binutils-2.34 they keep the
> NOBITS designation.
>
> I had thought NOLOAD implies NOBITS, but that doesn't seem to be the
> case anymore? I tinkered with module.lds a bit and noticed that the
> name of the section seems to matters. So this:
>
> SECTIONS {
> .plt (NOLOAD) : { BYTE(0) }
> .init.plt (NOLOAD) : { BYTE(0) }
> .text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
> + .test (NOLOAD) : { BYTE(0) }
> + .text.test (NOLOAD) : { BYTE(0) }
> + .plt.test (NOLOAD) : { BYTE(0) }
> }
>
> Yielded the following:
>
> [22] .plt PROGBITS0340 000e40 01 00
> WAX 0 0 1
> [23] .init.plt NOBITS 0341 000e41 01 00
> WA 0 0 1
> [24] .text.ftrace_trampoline PROGBITS0342 000e41 01
> 00 WAX 0 0 1
> [25] .test NOBITS 0343 000e42 01 00
> WA 0 0 1
> [26] .text.testPROGBITS0344 000e42 01 00
> WAX 0 0 1
> [27] .plt.test NOBITS 0345 000e43 01 00
> WA 0 0 1
>
> So ".plt" and any section starting with ".text" were automatically
> designated as SHT_PROGBITS and given the executable flag. It appears
> the NOLOAD directive has been ignored or overridden in the case of
> these sections. I am not sure if this is a bug in binutils or if this
> behavior is intentional.
>
> I tried to grok the changelog between 2.34 and 2.35 but could not find
> anything glaringly obvious that would cause this change. CC'ing the
> binutils mailing list, hopefully that's the right place to ask.
>
Please open a binutils bug with a testcase.
--
H.J.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Wed, Aug 12, 2020 at 12:40:05PM +0200, pet...@infradead.org wrote:
> On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
> > The module .lds has BYTE(0) in the section contents to prevent the
> > linker from pruning them entirely. The (NOLOAD) is there to ensure
> > that this byte does not end up in the .ko, which is more a matter of
> > principle than anything else, so we can happily drop that if it helps.
> >
> > However, this should only affect the PROGBITS vs NOBITS designation,
> > and so I am not sure whether it makes a difference.
> >
> > Depending on where the w^x check occurs, we might simply override the
> > permissions of these sections, and strip the writable permission if it
> > is set in the PLT handling init code, which manipulates the metadata
> > of all these 3 sections before the module space is vmalloc'ed.
>
> What's curious is that this seems the result of some recent binutils
> change. Every build with binutils-2.34 (or older) does not seem to
> generate these as WAX, but has the much more sensible WA.
>
> I suppose we can change the kernel check and 'allow' W^X for 0 sized
> sections, but I think we should still figure out why binutils-2.35 is
> now generating WAX sections all of a sudden, it might come bite us
> elsewhere.
Agreed, I think it's important to figure out what's going on here before we
try to bodge around it.
Adding Szabolcs, in case he has any ideas.
To save him reading the whole thread, here's a summary:
AArch64 kernel modules built with binutils 2.35 end up with a couple of
ELF sections marked as SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR:
[ 5] .plt PROGBITS 0388 01d000 08 00 WAX 0 0 1
[ 6] .init.plt NOBITS 0390 01d008 08 00 WA 0 0 1
[ 7] .text.ftrace_trampoline PROGBITS 0398 01d008 08 00 WAX 0
0 1
This results in the module being rejected by our loader, because we don't
permit writable, executable mappings.
Our linker script for these entries uses NOLOAD, so it's odd to see PROGBITS
appearing in the readelf output above (and older binutils emits NOBITS
sections). Anyway, here's the linker script:
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
.init.plt (NOLOAD) : { BYTE(0) }
.text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
}
It appears that the name of the section influences the behaviour, as
Jessica observed [1] that sections named .text.* end up with PROGBITS,
whereas random naming such as ".test" ends up with NOBITS, as before.
We've looked at the changelog between binutils 2.34 and 2.35, but nothing
stands out. Any clues? Is this intentional binutils behaviour?
Thanks,
Will
[1] https://lore.kernel.org/r/20200812114127.ga10...@linux-8ccs.fritz.box
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ pet...@infradead.org [12/08/20 12:40 +0200]:
On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote:
The module .lds has BYTE(0) in the section contents to prevent the
linker from pruning them entirely. The (NOLOAD) is there to ensure
that this byte does not end up in the .ko, which is more a matter of
principle than anything else, so we can happily drop that if it helps.
However, this should only affect the PROGBITS vs NOBITS designation,
and so I am not sure whether it makes a difference.
Depending on where the w^x check occurs, we might simply override the
permissions of these sections, and strip the writable permission if it
is set in the PLT handling init code, which manipulates the metadata
of all these 3 sections before the module space is vmalloc'ed.
What's curious is that this seems the result of some recent binutils
change. Every build with binutils-2.34 (or older) does not seem to
generate these as WAX, but has the much more sensible WA.
I suppose we can change the kernel check and 'allow' W^X for 0 sized
sections, but I think we should still figure out why binutils-2.35 is
now generating WAX sections all of a sudden, it might come bite us
elsewhere.
I have just tested with binutils-2.35 and am observing the same
behavior. Both .plt and .text.ftrace_trampoline end up with
SHT_PROGBITS and are marked 'WAX'. With binutils-2.34 they keep the
NOBITS designation.
I had thought NOLOAD implies NOBITS, but that doesn't seem to be the
case anymore? I tinkered with module.lds a bit and noticed that the
name of the section seems to matters. So this:
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
.init.plt (NOLOAD) : { BYTE(0) }
.text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
+ .test (NOLOAD) : { BYTE(0) }
+ .text.test (NOLOAD) : { BYTE(0) }
+ .plt.test (NOLOAD) : { BYTE(0) }
}
Yielded the following:
[22] .plt PROGBITS0340 000e40 01 00 WAX
0 0 1
[23] .init.plt NOBITS 0341 000e41 01 00 WA
0 0 1
[24] .text.ftrace_trampoline PROGBITS0342 000e41 01 00
WAX 0 0 1
[25] .test NOBITS 0343 000e42 01 00 WA
0 0 1
[26] .text.testPROGBITS0344 000e42 01 00 WAX
0 0 1
[27] .plt.test NOBITS 0345 000e43 01 00 WA
0 0 1
So ".plt" and any section starting with ".text" were automatically
designated as SHT_PROGBITS and given the executable flag. It appears
the NOLOAD directive has been ignored or overridden in the case of
these sections. I am not sure if this is a bug in binutils or if this
behavior is intentional.
I tried to grok the changelog between 2.34 and 2.35 but could not find
anything glaringly obvious that would cause this change. CC'ing the
binutils mailing list, hopefully that's the right place to ask.
Jessica
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Wed, Aug 12, 2020 at 10:56:56AM +0200, Ard Biesheuvel wrote: > The module .lds has BYTE(0) in the section contents to prevent the > linker from pruning them entirely. The (NOLOAD) is there to ensure > that this byte does not end up in the .ko, which is more a matter of > principle than anything else, so we can happily drop that if it helps. > > However, this should only affect the PROGBITS vs NOBITS designation, > and so I am not sure whether it makes a difference. > > Depending on where the w^x check occurs, we might simply override the > permissions of these sections, and strip the writable permission if it > is set in the PLT handling init code, which manipulates the metadata > of all these 3 sections before the module space is vmalloc'ed. What's curious is that this seems the result of some recent binutils change. Every build with binutils-2.34 (or older) does not seem to generate these as WAX, but has the much more sensible WA. I suppose we can change the kernel check and 'allow' W^X for 0 sized sections, but I think we should still figure out why binutils-2.35 is now generating WAX sections all of a sudden, it might come bite us elsewhere.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Tue, 11 Aug 2020 at 18:01, Jessica Yu wrote: > > +++ Mauro Carvalho Chehab [11/08/20 17:27 +0200]: > >Em Tue, 11 Aug 2020 16:55:24 +0200 > >pet...@infradead.org escreveu: > > > >> On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > >> > [33] .plt PROGBITS 0340 00035c80 > >> >0001 WAX 0 0 1 > >> > [34] .init.plt NOBITS 0341 00035c81 > >> >0001 WA 0 0 1 > >> > [35] .text.ftrace[...] PROGBITS 0342 00035c81 > >> >0001 WAX 0 0 1 > >> > >> .plt and .text.ftrace_tramplines are buggered. > >> > >> arch/arm64/kernel/module.lds even marks then as NOLOAD. > > > >Hmm... Shouldn't the code at module_enforce_rwx_sections() or at > >load_module() ignore such sections instead of just rejecting probing > >all modules? > > > >I mean, if the existing toolchain is not capable of excluding > >those sections, either the STRICT_MODULE_RWX hardening should be > >disabled, if a broken toolchain is detected or some runtime code > >should handle such sections on a different way. > > Hi Mauro, thanks for providing the readelf output. The sections marked 'WAX' > are indeed the reason why the module loader is rejecting them. > > Interesting, my cross-compiled modules do not have the executable flag - > > [38] .plt NOBITS 0340 00038fc0 >0001 WA 0 0 1 > [39] .init.plt NOBITS 0341 00038fc0 >0001 WA 0 0 1 > [40] .text.ftrace_tram NOBITS 0342 00038fc0 >0001 WA 0 0 1 > > ld version: > > GNU ld (GNU Binutils) 2.34 > Copyright (C) 2020 Free Software Foundation, Inc. > This program is free software; you may redistribute it under the terms of > the GNU General Public License version 3 or (at your option) a later > version. > > And gcc: > > aarch64-linux-gcc (GCC) 9.3.0 > Copyright (C) 2019 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR > PURPOSE. > > I'm a bit confused about what NOLOAD actually implies in this context. From > the > ld documentation - "The `(NOLOAD)' directive will mark a section to not be > loaded at run time." But these sections are marked SHF_ALLOC and are > referenced > to in the module plt code. Or does it just tell the linker to not initialize > it? > > Adding Ard to CC, I'm sure he'd know more about the section flag specifics. > The module .lds has BYTE(0) in the section contents to prevent the linker from pruning them entirely. The (NOLOAD) is there to ensure that this byte does not end up in the .ko, which is more a matter of principle than anything else, so we can happily drop that if it helps. However, this should only affect the PROGBITS vs NOBITS designation, and so I am not sure whether it makes a difference. Depending on where the w^x check occurs, we might simply override the permissions of these sections, and strip the writable permission if it is set in the PLT handling init code, which manipulates the metadata of all these 3 sections before the module space is vmalloc'ed.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Tue, Aug 11, 2020 at 07:59:12PM +0200, pet...@infradead.org wrote: > On Tue, Aug 11, 2020 at 06:01:35PM +0200, Jessica Yu wrote: > > > > > On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > > > > > [33] .plt PROGBITS 0340 00035c80 > > > > >0001 WAX 0 0 1 > > > > > [34] .init.plt NOBITS 0341 00035c81 > > > > >0001 WA 0 0 1 > > > > > [35] .text.ftrace[...] PROGBITS 0342 00035c81 > > > > >0001 WAX 0 0 1 > > > Interesting, my cross-compiled modules do not have the executable flag - > > > > [38] .plt NOBITS 0340 00038fc0 > > 0001 WA 0 0 1 > > [39] .init.plt NOBITS 0341 00038fc0 > > 0001 WA 0 0 1 > > [40] .text.ftrace_tram NOBITS 0342 00038fc0 > > 0001 WA 0 0 1 > > > I'm a bit confused about what NOLOAD actually implies in this context. From > > the > > ld documentation - "The `(NOLOAD)' directive will mark a section to not be > > loaded at run time." But these sections are marked SHF_ALLOC and are > > referenced > > to in the module plt code. Or does it just tell the linker to not > > initialize it? > > Yeah, that confusion is wide-spread, so much so that bfd-ld and gold, > both in bintils, had different behaviour at some point. > > Anyway, another clue is that your build has all NOBITS, while Mauro's > build has PROGBITS for the broken sections. > > Anyway, my gcc-10.1/binutils-2.34 cross tool chain (from k.org) > generates the same as Jessica's too. I wonder if binutils-2.35 is > wonky... When I use the Debian provided cross compiler which uses: binutils-aarch64-linux-gnu 2.35-1 I do indeed see the same thing Mauro does, which seems to suggest there's something really dodgy with that toolchain. Some tools person should have a look.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Tue, Aug 11, 2020 at 06:01:35PM +0200, Jessica Yu wrote: > > > On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > > > > [33] .plt PROGBITS 0340 00035c80 > > > >0001 WAX 0 0 1 > > > > [34] .init.plt NOBITS 0341 00035c81 > > > >0001 WA 0 0 1 > > > > [35] .text.ftrace[...] PROGBITS 0342 00035c81 > > > >0001 WAX 0 0 1 > Interesting, my cross-compiled modules do not have the executable flag - > > [38] .plt NOBITS 0340 00038fc0 > 0001 WA 0 0 1 > [39] .init.plt NOBITS 0341 00038fc0 > 0001 WA 0 0 1 > [40] .text.ftrace_tram NOBITS 0342 00038fc0 > 0001 WA 0 0 1 > I'm a bit confused about what NOLOAD actually implies in this context. From > the > ld documentation - "The `(NOLOAD)' directive will mark a section to not be > loaded at run time." But these sections are marked SHF_ALLOC and are > referenced > to in the module plt code. Or does it just tell the linker to not initialize > it? Yeah, that confusion is wide-spread, so much so that bfd-ld and gold, both in bintils, had different behaviour at some point. Anyway, another clue is that your build has all NOBITS, while Mauro's build has PROGBITS for the broken sections. Anyway, my gcc-10.1/binutils-2.34 cross tool chain (from k.org) generates the same as Jessica's too. I wonder if binutils-2.35 is wonky...
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Tue, Aug 11, 2020 at 06:01:35PM +0200, Jessica Yu wrote: > +++ Mauro Carvalho Chehab [11/08/20 17:27 +0200]: > > Em Tue, 11 Aug 2020 16:55:24 +0200 > > pet...@infradead.org escreveu: > > > > > On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > > > > [33] .plt PROGBITS 0340 00035c80 > > > >0001 WAX 0 0 1 > > > > [34] .init.plt NOBITS 0341 00035c81 > > > >0001 WA 0 0 1 > > > > [35] .text.ftrace[...] PROGBITS 0342 00035c81 > > > >0001 WAX 0 0 1 > > > > > > .plt and .text.ftrace_tramplines are buggered. > > > > > > arch/arm64/kernel/module.lds even marks then as NOLOAD. > > > > Hmm... Shouldn't the code at module_enforce_rwx_sections() or at > > load_module() ignore such sections instead of just rejecting probing > > all modules? > > > > I mean, if the existing toolchain is not capable of excluding > > those sections, either the STRICT_MODULE_RWX hardening should be > > disabled, if a broken toolchain is detected or some runtime code > > should handle such sections on a different way. > > Hi Mauro, thanks for providing the readelf output. The sections marked 'WAX' > are indeed the reason why the module loader is rejecting them. > > Interesting, my cross-compiled modules do not have the executable flag - > > [38] .plt NOBITS 0340 00038fc0 > 0001 WA 0 0 1 > [39] .init.plt NOBITS 0341 00038fc0 > 0001 WA 0 0 1 > [40] .text.ftrace_tram NOBITS 0342 00038fc0 > 0001 WA 0 0 1 FWIW, I also see the same output as you for both of the GCC 9 and Clang 11 builds I have kicking around, and there are no WAX sections in sight. Will
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Mauro Carvalho Chehab [11/08/20 17:27 +0200]: Em Tue, 11 Aug 2020 16:55:24 +0200 pet...@infradead.org escreveu: On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > [33] .plt PROGBITS 0340 00035c80 >0001 WAX 0 0 1 > [34] .init.plt NOBITS 0341 00035c81 >0001 WA 0 0 1 > [35] .text.ftrace[...] PROGBITS 0342 00035c81 >0001 WAX 0 0 1 .plt and .text.ftrace_tramplines are buggered. arch/arm64/kernel/module.lds even marks then as NOLOAD. Hmm... Shouldn't the code at module_enforce_rwx_sections() or at load_module() ignore such sections instead of just rejecting probing all modules? I mean, if the existing toolchain is not capable of excluding those sections, either the STRICT_MODULE_RWX hardening should be disabled, if a broken toolchain is detected or some runtime code should handle such sections on a different way. Hi Mauro, thanks for providing the readelf output. The sections marked 'WAX' are indeed the reason why the module loader is rejecting them. Interesting, my cross-compiled modules do not have the executable flag - [38] .plt NOBITS 0340 00038fc0 0001 WA 0 0 1 [39] .init.plt NOBITS 0341 00038fc0 0001 WA 0 0 1 [40] .text.ftrace_tram NOBITS 0342 00038fc0 0001 WA 0 0 1 ld version: GNU ld (GNU Binutils) 2.34 Copyright (C) 2020 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or (at your option) a later version. And gcc: aarch64-linux-gcc (GCC) 9.3.0 Copyright (C) 2019 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. I'm a bit confused about what NOLOAD actually implies in this context. From the ld documentation - "The `(NOLOAD)' directive will mark a section to not be loaded at run time." But these sections are marked SHF_ALLOC and are referenced to in the module plt code. Or does it just tell the linker to not initialize it? Adding Ard to CC, I'm sure he'd know more about the section flag specifics. Jessica
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
Em Tue, 11 Aug 2020 16:55:24 +0200 pet...@infradead.org escreveu: > On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > > [33] .plt PROGBITS 0340 00035c80 > >0001 WAX 0 0 1 > > [34] .init.plt NOBITS 0341 00035c81 > >0001 WA 0 0 1 > > [35] .text.ftrace[...] PROGBITS 0342 00035c81 > >0001 WAX 0 0 1 > > .plt and .text.ftrace_tramplines are buggered. > > arch/arm64/kernel/module.lds even marks then as NOLOAD. Hmm... Shouldn't the code at module_enforce_rwx_sections() or at load_module() ignore such sections instead of just rejecting probing all modules? I mean, if the existing toolchain is not capable of excluding those sections, either the STRICT_MODULE_RWX hardening should be disabled, if a broken toolchain is detected or some runtime code should handle such sections on a different way. Thanks, Mauro
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote: > [33] .plt PROGBITS 0340 00035c80 >0001 WAX 0 0 1 > [34] .init.plt NOBITS 0341 00035c81 >0001 WA 0 0 1 > [35] .text.ftrace[...] PROGBITS 0342 00035c81 >0001 WAX 0 0 1 .plt and .text.ftrace_tramplines are buggered. arch/arm64/kernel/module.lds even marks then as NOLOAD.
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
Hi Jessica,
Em Mon, 10 Aug 2020 17:06:50 +0200
Jessica Yu escreveu:
> +++ Jessica Yu [10/08/20 11:25 +0200]:
> >+++ Mauro Carvalho Chehab [08/08/20 10:12 +0200]:
> >[snip]
> >>Right now, what happens is:
> >>
> >># modprobe wlcore
> >>modprobe: ERROR: could not insert 'wlcore': Exec format error
> >>
> >>This seems to be failing for all modules, as doesn't show anything
> >>probed.
> >>
> >>Btw, IMO, it would be useful to have some pr_debug() infra in order to
> >>explain why insmod is failing, or to have more error codes used there,
> >>as nothing was printed at dmesg. That makes harder to debug issues
> >>there. I ended losing a lot of time yesterday rebuilding the Kernel
> >>and checking the FS, before deciding to add some printks inside the
> >>Kernel ;-)
> >>
> >>In order for modprobe to start working again, I had to apply this
> >>dirty hack:
> >>
> >>
> >>diff --git a/kernel/module.c b/kernel/module.c
> >>index 910a57640818..10d590dc48ad 100644
> >>--- a/kernel/module.c
> >>+++ b/kernel/module.c
> >>@@ -2051,11 +2051,12 @@ static int module_enforce_rwx_sections(Elf_Ehdr
> >>*hdr, Elf_Shdr *sechdrs,
> >>const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
> >>int i;
> >>
> >>+#if 0
> >>for (i = 0; i < hdr->e_shnum; i++) {
> >>if ((sechdrs[i].sh_flags & shf_wx) == shf_wx)
> >>return -ENOEXEC;
> >>}
> >>-
> >>+#endif
> >>return 0;
> >>}
> >>
>
> [ I somehow munged the To: header in the last mail. Sorry about that,
> it's fixed now. ]
>
> >All this hunk does it reject loading modules that have any sections
> >that have both the writable and executable flags. You're saying it's
> >happening for all modules on your setup - I am curious as to which
> >sections have both these flags
Yes, without the hack, all modules are rejected.
> what does readelf -S tell you?
$ readelf -S ./drivers/net/wireless/ti/wlcore/wlcore.ko
There are 54 section headers, starting at offset 0x8acd08:
Section Headers:
[Nr] Name Type Address Offset
Size EntSize Flags Link Info Align
[ 0] NULL
0 0 0
[ 1] .text PROGBITS 0040
0001ddb8 AX 0 0 8
[ 2] .rela.textRELA 004c3db8
0001b648 0018 I 51 1 8
[ 3] .text.unlikelyPROGBITS 0001ddf8
0388 AX 0 0 4
[ 4] .rela.text.u[...] RELA 004df400
03f0 0018 I 51 3 8
[ 5] __ksymtab PROGBITS 0001e180
0030 A 0 0 4
[ 6] .rela__ksymtabRELA 004df7f0
0120 0018 I 51 5 8
[ 7] __ksymtab_gpl PROGBITS 0001e1b0
0228 A 0 0 4
[ 8] .rela__ksymt[...] RELA 004df910
0cf0 0018 I 51 7 8
[ 9] .rodata PROGBITS 0001e3d8
309d A 0 0 8
[10] .rela.rodata RELA 004e0600
0d08 0018 I 51 9 8
[11] __ksymtab_strings PROGBITS 00021475
04eb 0001 AMS 0 0 1
[12] .rodata.str PROGBITS 00021960
160d 0001 AMS 0 0 1
[13] .rodata.str1.8PROGBITS 00022f70
6724 0001 AMS 0 0 8
[14] .modinfo PROGBITS 00029694
0238 A 0 0 1
[15] __param PROGBITS 000298d0
00c8 A 0 0 8
[16] .rela__param RELA 004e1308
01e0 0018 I 5115 8
[17] .altinstructions PROGBITS 00029998
0018 A 0 0 1
[18] .rela.altins[...] RELA 004e14e8
0060 0018 I 5117 8
[19] .eh_frame PROGBITS 000299b0
542c A 0 0 8
[20] .rela.eh_frameRELA 004e1548
2040 0018 I 5119 8
[21] .note.gnu.pr[...]
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Jessica Yu [10/08/20 11:25 +0200]:
+++ Mauro Carvalho Chehab [08/08/20 10:12 +0200]:
[snip]
Right now, what happens is:
# modprobe wlcore
modprobe: ERROR: could not insert 'wlcore': Exec format error
This seems to be failing for all modules, as doesn't show anything
probed.
Btw, IMO, it would be useful to have some pr_debug() infra in order to
explain why insmod is failing, or to have more error codes used there,
as nothing was printed at dmesg. That makes harder to debug issues
there. I ended losing a lot of time yesterday rebuilding the Kernel
and checking the FS, before deciding to add some printks inside the
Kernel ;-)
In order for modprobe to start working again, I had to apply this
dirty hack:
diff --git a/kernel/module.c b/kernel/module.c
index 910a57640818..10d590dc48ad 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2051,11 +2051,12 @@ static int module_enforce_rwx_sections(Elf_Ehdr *hdr,
Elf_Shdr *sechdrs,
const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
int i;
+#if 0
for (i = 0; i < hdr->e_shnum; i++) {
if ((sechdrs[i].sh_flags & shf_wx) == shf_wx)
return -ENOEXEC;
}
-
+#endif
return 0;
}
[ I somehow munged the To: header in the last mail. Sorry about that,
it's fixed now. ]
All this hunk does it reject loading modules that have any sections
that have both the writable and executable flags. You're saying it's
happening for all modules on your setup - I am curious as to which
sections have both these flags - what does readelf -S tell you?
Hmm, I was not able to reproduce this with a cross-compiled kernel
using the attached config (gcc 9.3.0 with vanilla v5.8 kernel). I am
curious if the failing sections are also SHF_ALLOC - in that case, the
code is doing what it is intended to do, which is rejecting loading
any modules with writable and executable sections. If the problematic
sections are *not* SHF_ALLOC, then we might be able to work around
that by ignoring non-SHF_ALLOC sections as they are not copied to the
final module location anyway.
Jessica
Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
+++ Mauro Carvalho Chehab [08/08/20 10:12 +0200]:
[snip]
Right now, what happens is:
# modprobe wlcore
modprobe: ERROR: could not insert 'wlcore': Exec format error
This seems to be failing for all modules, as doesn't show anything
probed.
Btw, IMO, it would be useful to have some pr_debug() infra in order to
explain why insmod is failing, or to have more error codes used there,
as nothing was printed at dmesg. That makes harder to debug issues
there. I ended losing a lot of time yesterday rebuilding the Kernel
and checking the FS, before deciding to add some printks inside the
Kernel ;-)
In order for modprobe to start working again, I had to apply this
dirty hack:
diff --git a/kernel/module.c b/kernel/module.c
index 910a57640818..10d590dc48ad 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2051,11 +2051,12 @@ static int module_enforce_rwx_sections(Elf_Ehdr *hdr,
Elf_Shdr *sechdrs,
const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
int i;
+#if 0
for (i = 0; i < hdr->e_shnum; i++) {
if ((sechdrs[i].sh_flags & shf_wx) == shf_wx)
return -ENOEXEC;
}
-
+#endif
return 0;
}
All this hunk does it reject loading modules that have any sections
that have both the writable and executable flags. You're saying it's
happening for all modules on your setup - I am curious as to which
sections have both these flags - what does readelf -S tell you?
Thanks,
Jessica
