From: Ard Biesheuvel <ard.biesheuvel@linaro.org> To: Ingo Molnar <mingo@kernel.org> Cc: Matt Fleming <matt@codeblueprint.co.uk>, Thomas Gleixner <tglx@linutronix.de>, "H. Peter Anvin" <hpa@zytor.com>, Matt Fleming <matt.fleming@intel.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>, "Lee, Chun-Yi" <jlee@suse.com>, Borislav Petkov <bp@suse.de>, Leif Lindholm <leif.lindholm@linaro.org>, Peter Jones <pjones@redhat.com>, James Bottomley <JBottomley@odin.com>, Matthew Garrett <mjg59@srcf.ucam.org>, Dave Young <dyoung@redhat.com>, "stable@vger.kernel.org" <stable@vger.kernel.org>, Linus Torvalds <torvalds@linux-foundation.org>, Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>, Denys Vlasenko <dvlasenk@redhat.com>, Brian Gerst <brgerst@gmail.com>, Andrew Morton <akpm@linux-foundation.org> Subject: Re: [PATCH 1/2] x86/efi: Map EFI memmap entries in-order at runtime Date: Mon, 28 Sep 2015 10:51:44 +0100 [thread overview] Message-ID: <CAKv+Gu-e5CRrJ1S7phXyWCUZ10ptukn0zr0wDGyX14H_SpEp1Q@mail.gmail.com> (raw) In-Reply-To: <20150928082245.GA28796@gmail.com> On 28 September 2015 at 09:22, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> On 27 September 2015 at 08:03, Ingo Molnar <mingo@kernel.org> wrote: >> > >> > * Matt Fleming <matt@codeblueprint.co.uk> 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, That is good to hear. > 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? > The 32-bit side essentially implements the old memmap only, which is the the bottom-up version. So old memmap will be implied by 32-bit but not set in the EFI flags, resulting in the reverse enumeration being used with the bottom-up mapping logic. The net result of that is that we create the same problem for 32-bit that we are trying to solve for 64-bit, i.e., the regions will end up in reverse order in the VA mapping. To deobfuscate this particular conditional, we could set EFI_OLD_MEMMAP unconditionally on 32-bit x86. Or we could reshuffle variables and conditionals in various other way. I am not convinced that the overall end result will be any better though. -- Ard.
WARNING: multiple messages have this Message-ID (diff)
From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> To: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>, Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>, Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>, Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, James Bottomley <JBottomley-wo1vFcy6AUs@public.gmane.org>, Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>, Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Denys Vlasenko <dvlasenk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Brian Gerst <brgerst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> Subject: Re: [PATCH 1/2] x86/efi: Map EFI memmap entries in-order at runtime Date: Mon, 28 Sep 2015 10:51:44 +0100 [thread overview] Message-ID: <CAKv+Gu-e5CRrJ1S7phXyWCUZ10ptukn0zr0wDGyX14H_SpEp1Q@mail.gmail.com> (raw) In-Reply-To: <20150928082245.GA28796-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> On 28 September 2015 at 09:22, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > > * 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, That is good to hear. > 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? > The 32-bit side essentially implements the old memmap only, which is the the bottom-up version. So old memmap will be implied by 32-bit but not set in the EFI flags, resulting in the reverse enumeration being used with the bottom-up mapping logic. The net result of that is that we create the same problem for 32-bit that we are trying to solve for 64-bit, i.e., the regions will end up in reverse order in the VA mapping. To deobfuscate this particular conditional, we could set EFI_OLD_MEMMAP unconditionally on 32-bit x86. Or we could reshuffle variables and conditionals in various other way. I am not convinced that the overall end result will be any better though. -- Ard.
next prev parent reply other threads:[~2015-09-28 9:51 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-09-25 22:02 [GIT PULL 0/2] EFI urgent fixes Matt Fleming 2015-09-25 22:02 ` Matt Fleming 2015-09-25 22:02 ` [PATCH 1/2] x86/efi: Map EFI memmap entries in-order at runtime Matt Fleming 2015-09-25 22:02 ` Matt Fleming 2015-09-26 5:56 ` Ingo Molnar 2015-09-26 5:56 ` Ingo Molnar 2015-09-26 6:44 ` Ard Biesheuvel 2015-09-26 6:44 ` Ard Biesheuvel 2015-09-26 13:43 ` Matt Fleming 2015-09-27 7:03 ` Ingo Molnar 2015-09-27 7:03 ` Ingo Molnar 2015-09-28 6:49 ` Ard Biesheuvel 2015-09-28 8:22 ` Ingo Molnar 2015-09-28 8:22 ` Ingo Molnar 2015-09-28 9:51 ` Ard Biesheuvel [this message] 2015-09-28 9:51 ` Ard Biesheuvel 2015-09-29 9:12 ` Ingo Molnar 2015-09-29 10:41 ` Ard Biesheuvel 2015-09-29 14:18 ` Matt Fleming 2015-09-29 14:18 ` Matt Fleming 2015-09-29 13:52 ` Matt Fleming 2015-09-29 13:52 ` Matt Fleming 2015-09-26 17:01 ` Andy Lutomirski 2015-09-26 17:01 ` Andy Lutomirski 2015-09-26 17:20 ` H. Peter Anvin 2015-09-26 18:15 ` Ard Biesheuvel 2015-09-26 18:15 ` Ard Biesheuvel 2015-09-26 19:49 ` H. Peter Anvin 2015-09-26 19:57 ` Matt Fleming 2015-09-26 20:09 ` Ard Biesheuvel 2015-09-26 20:09 ` Ard Biesheuvel 2015-09-26 20:19 ` H. Peter Anvin 2015-09-27 16:30 ` Andy Lutomirski 2015-09-27 18:06 ` Matthew Garrett 2015-09-27 18:06 ` Matthew Garrett 2015-09-28 6:16 ` Ingo Molnar 2015-09-28 6:16 ` Ingo Molnar 2015-09-28 6:41 ` Matthew Garrett 2015-09-29 21:58 ` Laszlo Ersek 2015-09-29 21:58 ` Laszlo Ersek 2015-09-30 9:30 ` Ard Biesheuvel 2015-09-30 16:43 ` Andy Lutomirski 2015-09-30 16:43 ` Andy Lutomirski 2015-09-30 17:24 ` James Bottomley 2015-09-30 17:24 ` James Bottomley 2015-09-30 17:24 ` James Bottomley 2015-09-30 0:54 ` H. Peter Anvin 2015-09-30 0:54 ` H. Peter Anvin 2015-09-26 19:55 ` Matt Fleming 2015-09-26 19:55 ` Matt Fleming 2015-09-27 6:50 ` Ingo Molnar 2015-10-01 12:48 ` [tip:core/urgent] x86/efi: Fix boot crash by mapping EFI memmap entries bottom-up at runtime, instead of top-down tip-bot for Matt Fleming 2015-10-02 9:44 ` Matt Fleming 2015-09-25 22:02 ` [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions Matt Fleming 2015-09-25 22:02 ` Matt Fleming 2015-09-26 6:01 ` Ingo Molnar 2015-09-26 6:01 ` Ingo Molnar 2015-09-26 7:08 ` Ard Biesheuvel 2015-09-26 7:08 ` Ard Biesheuvel 2015-09-27 7:06 ` Ingo Molnar 2015-09-27 7:06 ` Ingo Molnar 2015-09-27 10:40 ` Borislav Petkov 2015-09-28 6:20 ` Ingo Molnar 2015-09-29 9:31 ` Dave Young 2015-09-29 10:24 ` Borislav Petkov 2015-09-29 14:36 ` Matt Fleming 2015-09-29 14:36 ` Matt Fleming 2015-09-30 0:56 ` H. Peter Anvin 2015-09-30 0:56 ` H. Peter Anvin 2015-09-30 8:33 ` Borislav Petkov 2015-09-30 8:33 ` Borislav Petkov 2015-09-30 1:03 ` H. Peter Anvin 2015-09-30 1:16 ` Andy Lutomirski 2015-09-30 1:19 ` H. Peter Anvin 2015-09-30 4:24 ` Ard Biesheuvel 2015-09-30 4:24 ` Ard Biesheuvel 2015-10-01 10:44 ` Ingo Molnar 2015-10-01 12:49 ` [tip:core/urgent] arm64/efi: Fix boot crash by not padding " tip-bot for Ard Biesheuvel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAKv+Gu-e5CRrJ1S7phXyWCUZ10ptukn0zr0wDGyX14H_SpEp1Q@mail.gmail.com \ --to=ard.biesheuvel@linaro.org \ --cc=JBottomley@odin.com \ --cc=akpm@linux-foundation.org \ --cc=bp@alien8.de \ --cc=bp@suse.de \ --cc=brgerst@gmail.com \ --cc=dvlasenk@redhat.com \ --cc=dyoung@redhat.com \ --cc=hpa@zytor.com \ --cc=jlee@suse.com \ --cc=leif.lindholm@linaro.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=matt.fleming@intel.com \ --cc=matt@codeblueprint.co.uk \ --cc=mingo@kernel.org \ --cc=mjg59@srcf.ucam.org \ --cc=pjones@redhat.com \ --cc=stable@vger.kernel.org \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.