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;
MIME-Version: 1.0
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>
In-Reply-To: <20200811160134.GA13652@linux-8ccs>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 12 Aug 2020 10:56:56 +0200
Message-ID: <CAMj1kXF8fm=9CdQykqDbgYCJSP88ezMs3EOosCW+SDi+Lve0zg@mail.gmail.com>
Subject: Re: [PATCH v2] module: Harden STRICT_MODULE_RWX
To:     Jessica Yu <jeyu@kernel.org>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Peter Zijlstra <peterz@infradead.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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 11 Aug 2020 at 18:01, Jessica Yu <jeyu@kernel.org> wrote:
>
> +++ Mauro Carvalho Chehab [11/08/20 17:27 +0200]:
> >Em Tue, 11 Aug 2020 16:55:24 +0200
> >peterz@infradead.org escreveu:
> >
> >> On Tue, Aug 11, 2020 at 04:34:27PM +0200, Mauro Carvalho Chehab wrote:
> >> >   [33] .plt              PROGBITS         0000000000000340  00035c80
> >> >        0000000000000001  0000000000000000 WAX       0     0     1
> >> >   [34] .init.plt         NOBITS           0000000000000341  00035c81
> >> >        0000000000000001  0000000000000000  WA       0     0     1
> >> >   [35] .text.ftrace[...] PROGBITS         0000000000000342  00035c81
> >> >        0000000000000001  0000000000000000 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           0000000000000340  00038fc0
>        0000000000000001  0000000000000000  WA       0     0     1
>   [39] .init.plt         NOBITS           0000000000000341  00038fc0
>        0000000000000001  0000000000000000  WA       0     0     1
>   [40] .text.ftrace_tram NOBITS           0000000000000342  00038fc0
>        0000000000000001  0000000000000000  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.