Messages in this thread Patch in this message | | | Date | Mon, 31 Aug 2020 11:46:51 +0200 | From | Jessica Yu <> | Subject | 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
| |