Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 12 Aug 2020 13:41:27 +0200
From:   Jessica Yu <jeyu@kernel.org>
To:     peterz@infradead.org
Cc:     Ard Biesheuvel <ardb@kernel.org>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Kees Cook <keescook@chromium.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Mark Rutland <mark.rutland@arm.com>, binutils@sourceware.org
Subject: Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
Message-ID: <20200812114127.GA10824@linux-8ccs.fritz.box>
References: <20200403171303.GK20760@hirez.programming.kicks-ass.net>
 <20200808101222.5103093e@coco.lan>
 <20200810092523.GA8612@linux-8ccs>
 <20200810150647.GB8612@linux-8ccs>
 <20200811163427.6edbf343@coco.lan>
 <20200811145524.GE2674@hirez.programming.kicks-ass.net>
 <20200811172738.2d632a09@coco.lan>
 <20200811160134.GA13652@linux-8ccs>
 <CAMj1kXF8fm=9CdQykqDbgYCJSP88ezMs3EOosCW+SDi+Lve0zg@mail.gmail.com>
 <20200812104005.GN2674@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20200812104005.GN2674@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+++ peterz@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              PROGBITS        0000000000000340 000e40 000001 00 WAX  0   0  1
  [23] .init.plt         NOBITS          0000000000000341 000e41 000001 00  WA  0   0  1
  [24] .text.ftrace_trampoline PROGBITS        0000000000000342 000e41 000001 00 WAX  0   0  1
  [25] .test             NOBITS          0000000000000343 000e42 000001 00  WA  0   0  1
  [26] .text.test        PROGBITS        0000000000000344 000e42 000001 00 WAX  0   0  1
  [27] .plt.test         NOBITS          0000000000000345 000e43 000001 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