Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings

[Ard Biesheuvel]

On Fri, 23 Sept 2022 at 15:58, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 9/23/22 02:49, Ard Biesheuvel wrote:
> > (cc Kees)
> >
> > On Fri, 23 Sept 2022 at 09:00, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >>
> >> On Fri, Sep 23, 2022 at 12:08:57AM +0200, Ard Biesheuvel wrote:
> >>> On Thu, 22 Sept 2022 at 21:32, Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> wrote:
> >>>>
> >>>> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> >>>>
> >>>> I'm planning on sticking this in x86/mm so that it goes upstream
> >>>> along with the W+X detection code.
> >>>>
> >>>> --
> >>>>
> >>>> A recent x86/mm change warns and refuses to create W+X mappings.
> >>>>
> >>>> The 32-bit EFI code tries to create such a mapping and trips over
> >>>> the new W+X refusal.
> >>>>
> >>>> Make the EFI_RUNTIME_SERVICES_CODE mapping read-only to fix it.
> >>>>
> >>>
> >>> This is not safe. EFI_RUNTIME_SERVICES_CODE covers both .text and
> >>> .data sections of the EFI runtime PE/COFF executables in memory, so
> >>> you are essentially making .data and .bss read-only. (Whether those
> >>> executables actually modify their .data and .bss at runtime is a
> >>> different matter, but the point is that it used to be possible)
> >>>
> >>> More recent firmwares may provide a 'memory attributes table'
> >>> separately which describes the individual sections, but older 32-bit
> >>> firmwares are not even built with 4k section alignment, so code and
> >>> data may share a single page. Note that we haven't wired up this
> >>> memory attributes table on i386 at the moment, and I seriously doubt
> >>> that 32-bit firmware in the field exposes it.
> >>>
> >>> Can we just turn off this feature for 32-bit?
> >>
> >> Goodie; some seriously security minded people who did that EFI turd :/
> >
> > To be fair, most people tended to care more about memory footprint
> > than about security at the time. And I don't recall a lot of
> > enthusiasm in the Linux community either for rounding up kernel
> > sections so they could be mapped with W^X permissions. And without
> > PAE, all memory is executable anyway.
> >
> >> Let's just heap it on the pile of 32bit sucks and should not be
> >> considered a security target anymore and indeed kill this feature.
> >>
> >
> > I take it this issue is triggered by the fact that i386 maps the EFI
> > runtime regions into the kernel page tables, and are therefore always
> > mapped, right? If anyone cares enough about this to go and fix it, we
> > could switch to the approach we use everywhere else, i.e., treat EFI
> > memory as user space mappings, and activate them only while a runtime
> > service is in progress.
> >
> > But frankly, why would anyone still be running this? With the EFI
> > mixed mode support, only systems with CPUs that don't actually
> > implement long mode still need this, and I am skeptical that such
> > deployments would use recent kernels.
>
> It is supported, thus I run qemu tests for it. That is the whole point
> of testing, after all.

I completely agree with that, and I think all the testing you do is
extremely valuable.

> If PAE (assuming that is what you are talking about)

Not at all - I was referring to i386 support in general.

I was basically making the point that we still support i386 without
PAE (which is a prerequisite for supporting non-executable mappings),
and if we are going to be pedantic about security on this
architecture, we should probably make PAE mandatory as well.

If we are ok with the current state, enabling this permission check on
i386 makes no sense.

> is no longer supported or supportable, its support should be
> removed. If so, I'll be very happy to stop testing it.
>

I'd say there are better ways to spend those cycles, but for the time
being, I think we should continue testing it, as otherwise it will
just bit rot.