Re: [PATCH 1/2] x86/efi: Map EFI memmap entries in-order at runtime

[Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 27 September 2015 at 08:03, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> >
> [...]
> >> [...] The actual virtual addresses we pick are exactly the same with the two
> >> patches.
> >
> > So I'm NAK-ing this for now:
> >
> >  - The code is it reads today pretends to be an 'allocator'. It is _NOT_ an
> >    allocator, because all the sections have already been determined by the
> >    firmware, and, as we just learned the hard way, we do not want to deviate from
> >    that! There's nothing to 'allocate'!
> >
> >    What these patches seem to implement is an elaborate 'allocator' that ends up
> >    doing nothing on 'new 64-bit' ...
> >
> >  - The 32-bit and 64-bit and 'old_mmap' asymmetries:
> >
> >         if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
> >
> >    seem fragile and nonsensical. The question is: is it possible for the whole EFI
> >    image to be larger than a couple of megabytes? If not then 32-bit should just
> >    mirror the firmware layout as well, and if EFI_OLD_MEMMAP does anything
> >    differently from this _obvious_ 1:1 mapping of the EFI memory offsets then it's
> >    not worth keeping as a legacy, because there's just nothing better than
> >    mirroring the firmware layout.
> >
> > My suggestion would be to just 1:1 map what the EFI tables describe, modulo the
> > single absolute offset by which we shift the whole thing to a single base.
> >
> > Is there any technical reason why we'd want to deviate from that? Gigabytes of
> > tables or gigabytes of holes that 32-bit cannot handle? Firmware that wants an OS
> > layout that differs from the firmware layout?
> >
> 
> The combined EFI_MEMORY_RUNTIME regions could span the entire 1:1 addressable PA 
> space. They usually don't but it is a possibility, which means 32-bit will not 
> generally be able to support this approach. [...]

Ok, that's a good argument which invalidates my NAK.

> [...] For 64-bit ARM, there are some minor complications when the base of RAM is 
> up very high in physical memory, but we already fixed that for the boot time ID 
> map and for KVM.
> 
> > Also, nobody seems to be asking the obvious hardware compatibility question 
> > when trying to implement a standard influenced in great part by an entity that 
> > is partly ignorant of and partly hostile to Linux: how does Windows map the 
> > EFI sections, under what OSs are these firmware versions tested? I suspect no 
> > firmware is released that crashes on bootup on all OSs that can run on that 
> > hardware, right?
> 
> Interestingly, it was the other way around this time. The engineers that 
> implemented this feature for EDK2 could not boot Windows 8 anymore, because it 
> supposedly maps the regions in reverse order as well (and MS too will need to 
> backport a fix that inverts the mapping order). The engineers also tested 
> Linux/x86, by means of a SUSE installer image, which booted fine, most likely 
> due to the fact that it is an older version which still uses the old memmap 
> layout.

That's nice to hear!

> My concern with all of this is that this security feature will become an obscure 
> opt-in feature rather than something UEFIv2.5 firmware implementations can 
> enable by default.

Ok, so I think the patches are mostly fine after all, except that I don't think 
the condition on 64-bit makes any sense:

+       if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {

I can see us being nervous wrt. backported patches, but is there any strong reason 
to not follow this up with a third (non-backported) patch that changes this to:

+       if (!efi_enabled(EFI_OLD_MEMMAP)) {

for v4.4?

Thanks,

	Ingo