Re: [PATCH v2] module: Harden STRICT_MODULE_RWX

From: Jessica Yu
Date: Wed Aug 12 2020 - 12:00:25 EST

+++ 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, peterz@xxxxxxxxxxxxx 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

[ 5] .plt PROGBITS 0000000000000388 01d000 000008 00 WAX 0 0 1
[ 6] .init.plt NOBITS 0000000000000390 01d008 000008 00 WA 0 0 1
[ 7] .text.ftrace_trampoline PROGBITS 0000000000000398 01d008 000008 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:

.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;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..