Date:   Wed, 15 Mar 2023 13:57:33 -0400
From:   Peter Jones <pjones@redhat.com>
To:     Andy Lutomirski <luto@kernel.org>
Cc:     Evgeniy Baskov <baskov@ispras.ru>,
        Ard Biesheuvel <ardb@kernel.org>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        Gerd Hoffmann <kraxel@redhat.com>,
        "Limonciello, Mario" <mario.limonciello@amd.com>,
        joeyli <jlee@suse.com>, lvc-project@linuxtesting.org,
        the arch/x86 maintainers <x86@kernel.org>,
        linux-efi@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH v5 00/27] x86_64: Improvements at compressed kernel stage
Message-ID: <8493680a-0bad-43de-a7a0-caa48e430139@uncooperative.org>
References: <cover.1678785672.git.baskov@ispras.ru>
 <d575db7f-bad3-477e-a501-19d2d84527cd@app.fastmail.com>
 <ea1b6e36-c434-49e9-bede-b4bd2b41868d@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ea1b6e36-c434-49e9-bede-b4bd2b41868d@app.fastmail.com>
Precedence: bulk

On Tue, Mar 14, 2023 at 04:20:43PM -0700, Andy Lutomirski wrote:
> 
> 
> On Tue, Mar 14, 2023, at 2:23 PM, Andy Lutomirski wrote:
> > On Tue, Mar 14, 2023, at 3:13 AM, Evgeniy Baskov wrote:
> >>
> >> Kernel is made to be more compatible with PE image specification [3],
> >> allowing it to be successfully loaded by stricter PE loader
> >> implementations like the one from [2]. There is at least one
> >> known implementation that uses that loader in production [4].
> >> There are also ongoing efforts to upstream these changes.
> >
> > Can you clarify 
> 
> Sorry, lost part of a sentence.  Can you clarify in what respect the loader is stricter?
> 
> 
> Anyway, I did some research.  I found:
> 
> https://github.com/rhboot/shim/pull/459/commits/99a8d19326f69665e0b86bcfa6a59d554f662fba
> 
> which gives a somewhat incoherent-sounding description in which
> setting EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT apparently enables
> allocating memory that isn't RWX.  But this seems odd
> EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT is a property of the EFI
> *program*, not the boot services implementation.

Well, "is this binary compatible" is a property of the program, yes.
It's up to the loader to decide if it *cares*, and the compatibility
flag allows it to do that.

> And I'd be surprised if a flag on the application changes the behavior
> of boot services, but, OTOH, this is Microsoft.

There has been discussion of implementing a compatibility mode that
allows you to enable NX support by default, but only breaks the old
assumptions that the stack and memory allocations will be executable if
the flag is set, so that newer OSes get the mitigations we need, but
older OSes still work.  I don't think anyone has actually implemented
this *yet*, but some hardware vendors have made noises that sound like
they may intend to.  (I realize that sounds cagey as hell.  Sorry.)

Currently I think the only shipping systems that implement
NX-requirements are from Microsoft - the Surface product line and
Windows Dev Kit - and they don't allow you to disable it at all.  Other
vendors have produced firmware that isn't shipping yet (I *think*) that
has it as a setting in the firmware menu, and they're looking to move to
enabling it by default on some product lines.  We'd like to not be left
behind.

> And the PE 89 spec does say that
> EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT means "Image is NX compatible"
> and that is the sole mention of NX in the document.

Yeah, the PE spec is not very good in a lot of ways unrelated to how
abominable the thing it's describing is.

> And *this* seems to be the actual issue:
> 
> https://github.com/rhboot/shim/pull/459/commits/825d99361b4aaa16144392dc6cea43e24c8472ae
> 
> I assume that MS required this change as a condition for signing, but
> what do I know?

Yes, they have, but it's not as if they did it in a vacuum.  I think the
idea was originally Kees Cook's actually, and there's been a significant
effort on the firmware and bootloader side to enable it.  And there's
good reason to do this, too - more and more of this surface is being
attacked, and recently we've seen the first "bootkit" that actually
includes a Secure Boot breakout in the wild: https://www.welivesecurity.com/2023/03/01/blacklotus-uefi-bootkit-myth-confirmed/

While that particular malware (somewhat ironically) only uses code
developed for linux systems *after* the exploit, it could easily have
gone the other way, and we're definitely a target here.  We need NX in
our boot path, and soon.

> Anyway, the rules appear to be that the PE sections
> must not be both W and X at the same size.  (For those who are
> familiar with the abomination known as ELF but not with the
> abomination known as PE, a "section" is a range in the file that gets
> mapped into memory.  Like a PT_LOAD segment in ELF.)
> 
> Now I don't know whether anything prevents us from doing something
> awful like mapping the EFI stuf RX and then immediately *re*mapping
> everything RWX.  (Not that I'm seriously suggesting that.)

Once we've taken over paging, nothing stops us at all.  Until then,
SetMemoryAttributes() (which is more or less mprotect()) might prevent
it.

> And it's not immediately clear to me how the rest of this series fits
> in, what this has to do with the identity map, etc.

I'll let Evgeniy address that and the rest of this.

-- 
        Peter