Re: [PATCH v2] module: Harden STRICT_MODULE_RWX

[Masahiro Yamada]

On Tue, Aug 25, 2020 at 12:24 AM Jessica Yu <jeyu@xxxxxxxxxx> wrote:
>
> +++ Ard Biesheuvel [22/08/20 15:47 +0200]:
> >(+ Masahiro)
> >
> >On Fri, 21 Aug 2020 at 14:30, Will Deacon <will@xxxxxxxxxx> wrote:
> >>
> >> On Fri, Aug 21, 2020 at 02:27:05PM +0200, Ard Biesheuvel wrote:
> >> > On Fri, 21 Aug 2020 at 14:20, Will Deacon <will@xxxxxxxxxx> wrote:
> >> > >
> >> > > On Thu, Aug 13, 2020 at 03:07:13PM +0200, Ard Biesheuvel wrote:
> >> > > > On Thu, 13 Aug 2020 at 15:04, Jessica Yu <jeyu@xxxxxxxxxx> wrote:
> >> > > > >
> >> > > > > +++ Ard Biesheuvel [13/08/20 10:36 +0200]:
> >> > > > > >On Wed, 12 Aug 2020 at 22:00, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >> > > > > >>
> >> > > > > >> On Wed, Aug 12, 2020 at 06:37:57PM +0200, Ard Biesheuvel wrote:
> >> > > > > >> > I know there is little we can do at this point, apart from ignoring
> >> > > > > >> > the permissions - perhaps we should just defer the w^x check until
> >> > > > > >> > after calling module_frob_arch_sections()?
> >> > > > > >>
> >> > > > > >> My earlier suggestion was to ignore it for 0-sized sections.
> >> > > > > >
> >> > > > > >Only they are 1 byte sections in this case.
> >> > > > > >
> >> > > > > >We override the sh_type and sh_flags explicitly for these sections at
> >> > > > > >module load time, so deferring the check seems like a reasonable
> >> > > > > >alternative to me.
> >> > > > >
> >> > > > > So module_enforce_rwx_sections() is already called after
> >> > > > > module_frob_arch_sections() - which really baffled me at first, since
> >> > > > > sh_type and sh_flags should have been set already in
> >> > > > > module_frob_arch_sections().
> >> > > > >
> >> > > > > I added some debug prints to see which section the module code was
> >> > > > > tripping on, and it was .text.ftrace_trampoline. See this snippet from
> >> > > > > arm64's module_frob_arch_sections():
> >> > > > >
> >> > > > >                 else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
> >> > > > >                          !strcmp(secstrings + sechdrs[i].sh_name,
> >> > > > >                                  ".text.ftrace_trampoline"))
> >> > > > >                         tramp = sechdrs + i;
> >> > > > >
> >> > > > > Since Mauro's config doesn't have CONFIG_DYNAMIC_FTRACE enabled, tramp
> >> > > > > is never set here and the if (tramp) check at the end of the function
> >> > > > > fails, so its section flags are never set, so they remain WAX and fail
> >> > > > > the rwx check.
> >> > > >
> >> > > > Right. Our module.lds does not go through the preprocessor, so we
> >> > > > cannot add the #ifdef check there currently. So we should either drop
> >> > > > the IS_ENABLED() check here, or simply rename the section, dropping
> >> > > > the .text prefix (which doesn't seem to have any significance outside
> >> > > > this context)
> >> > > >
> >> > > > I'll leave it to Will to make the final call here.
> >> > >
> >> > > Why don't we just preprocess the linker script, like we do for the main
> >> > > kernel?
> >> > >
> >> >
> >> > That should work as well, I just haven't checked how straight-forward
> >> > it is to change that.
> >>
> >> Ok, if it's _not_ straightforward, then let's just drop the IS_ENABLED()
> >> altogether.
> >>
> >
> >I played around with this for a while, but failed to get Kbuild to
> >instantiate $(objtree)/arch/arm64/kernel/module.lds based on
> >$(srctree)/arch/arm64/kernel/module.lds.S and the cpp_lds_S rule.
> >Perhaps Masahiro has any suggestions here? Otherwise, let's just drop
> >the IS_ENABLED() check for now.
>
> I also tinkered around a bit and was able to generate
> $(objtree)/arch/arm64/kernel/module.lds based on
> $(srctree)/arch/arm64/kernel/module.lds.S only if I specified the
> former as the make target directly. Correct me if I'm wrong, but I
> guess this might be because the single build targets would utilize
> scripts/Makefile.build (where the cpp_lds_S rule is defined) while the
> module-related Makefiles don't seem to support .lds.S -> .lds in
> general.. Masahiro, how easy would it be to extend .lds.S -> .lds
> support to module linker scripts as well?
>
> Thanks,
>
> Jessica



If you want to generate
$(objtree)/arch/arm64/kernel/module.lds,
you need to tell it to the build system.

The following seems to work,
but please do NOT do this:


----------------->8--------------------
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index b45f0124cc16..4ae398c21111 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -116,7 +116,7 @@ endif
 CHECKFLAGS     += -D__aarch64__

 ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
-KBUILD_LDS_MODULE      += $(srctree)/arch/arm64/kernel/module.lds
+KBUILD_LDS_MODULE      += arch/arm64/kernel/module.lds
 endif

 ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index a561cbb91d4d..693797e6db01 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -71,3 +71,5 @@ extra-y                                       +=
$(head-y) vmlinux.lds
 ifeq ($(CONFIG_DEBUG_EFI),y)
 AFLAGS_head.o += -DVMLINUX_PATH="\"$(realpath $(objtree)/vmlinux)\""
 endif
+
+always-$(CONFIG_MODULES) += module.lds
diff --git a/arch/arm64/kernel/module.lds b/arch/arm64/kernel/module.lds.S
similarity index 100%
rename from arch/arm64/kernel/module.lds
rename to arch/arm64/kernel/module.lds.S
----------------->8--------------------





The difference between
arch/arm64/kernel/vmlinux.lds and
arch/arm64/kernel/module.lds is that
we must keep the latter for external
module builds.

'make clean' cannot remove any
build artifact used for external modules,
but 'make mrproper' must remove them.

Let me think about it.



-- 
Best Regards
Masahiro Yamada