All of lore.kernel.org
 help / color / mirror / Atom feed
From: tip-bot for Naoya Horiguchi <tipbot@zytor.com>
To: linux-tip-commits@vger.kernel.org
Cc: j-nomura@ce.jp.nec.com, bhe@redhat.com,
	linux-kernel@vger.kernel.org, tglx@linutronix.de,
	mingo@kernel.org, hpa@zytor.com, keescook@chromium.org,
	n-horiguchi@ah.jp.nec.com, torvalds@linux-foundation.org,
	thgarnie@google.com, ard.biesheuvel@linaro.org,
	peterz@infradead.org, matt@codeblueprint.co.uk
Subject: [tip:x86/boot] x86/boot/KASLR: Work around firmware bugs by excluding EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice
Date: Thu, 31 Aug 2017 13:00:53 -0700	[thread overview]
Message-ID: <tip-0982adc746736a313dac9cb8cc936ca51ca3741a@git.kernel.org> (raw)
In-Reply-To: <20170828074444.GC23181@hori1.linux.bs1.fc.nec.co.jp>

Commit-ID:  0982adc746736a313dac9cb8cc936ca51ca3741a
Gitweb:     http://git.kernel.org/tip/0982adc746736a313dac9cb8cc936ca51ca3741a
Author:     Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
AuthorDate: Mon, 28 Aug 2017 16:30:59 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 31 Aug 2017 12:00:35 +0200

x86/boot/KASLR: Work around firmware bugs by excluding EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice

There's a potential bug in how we select the KASLR kernel address n
the early boot code.

The KASLR boot code currently chooses the kernel image's physical memory
location from E820_TYPE_RAM regions by walking over all e820 entries.

E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and EFI_BOOT_SERVICES_DATA
as well, so those regions can end up hosting the kernel image. According to
the UEFI spec, all memory regions marked as EfiBootServicesCode and
EfiBootServicesData are available as free memory after the first call
to ExitBootServices(). I.e. so such regions should be usable for the
kernel, per spec.

In real life however, we have workarounds for broken x86 firmware,
where we keep such regions reserved until SetVirtualAddressMap() is done.

See the following code in should_map_region():

	static bool should_map_region(efi_memory_desc_t *md)
	{
		...
		/*
		 * Map boot services regions as a workaround for buggy
		 * firmware that accesses them even when they shouldn't.
		 *
		 * See efi_{reserve,free}_boot_services().
		 */
		if (md->type =3D=3D EFI_BOOT_SERVICES_CODE ||
			md->type =3D=3D EFI_BOOT_SERVICES_DATA)
				return false;

This workaround suppressed a boot crash, but potential issues still
remain because no one prevents the regions from overlapping with kernel
image by KASLR.

So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
chosen as kernel memory for the workaround to work fine.

Furthermore, EFI_LOADER_{CODE|DATA} regions are also excluded because
they can be used after ExitBootServices() as defined in EFI spec.

As a result, we choose kernel address only from EFI_CONVENTIONAL_MEMORY
which is the only memory type we know to be safely free.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Junichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Garnier <thgarnie@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: fanc.fnst@cn.fujitsu.com
Cc: izumi.taku@jp.fujitsu.com
Link: http://lkml.kernel.org/r/20170828074444.GC23181@hori1.linux.bs1.fc.nec.co.jp
[ Rewrote/fixed/clarified the changelog and the in code comments. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/kaslr.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 7de23bb..17818ba 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -597,19 +597,41 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 	for (i = 0; i < nr_desc; i++) {
 		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
 		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
-			region.start = md->phys_addr;
-			region.size = md->num_pages << EFI_PAGE_SHIFT;
-			process_mem_region(&region, minimum, image_size);
 			efi_mirror_found = true;
-
-			if (slot_area_index == MAX_SLOT_AREA) {
-				debug_putstr("Aborted EFI scan (slot_areas full)!\n");
-				break;
-			}
+			break;
 		}
 	}
 
-	return efi_mirror_found;
+	for (i = 0; i < nr_desc; i++) {
+		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
+
+		/*
+		 * Here we are more conservative in picking free memory than
+		 * the EFI spec allows:
+		 *
+		 * According to the spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
+		 * free memory and thus available to place the kernel image into,
+		 * but in practice there's firmware where using that memory leads
+		 * to crashes.
+		 *
+		 * Only EFI_CONVENTIONAL_MEMORY is guaranteed to be free.
+		 */
+		if (md->type != EFI_CONVENTIONAL_MEMORY)
+			continue;
+
+		if (efi_mirror_found &&
+		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
+			continue;
+
+		region.start = md->phys_addr;
+		region.size = md->num_pages << EFI_PAGE_SHIFT;
+		process_mem_region(&region, minimum, image_size);
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+			break;
+		}
+	}
+	return true;
 }
 #else
 static inline bool

      parent reply	other threads:[~2017-08-31 20:06 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-24 10:33 [PATCH v4] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Naoya Horiguchi
2017-08-28  6:59 ` Baoquan He
2017-08-28  7:44   ` [PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_* " Naoya Horiguchi
2017-08-28  7:55     ` Baoquan He
2017-08-31 20:00     ` tip-bot for Naoya Horiguchi [this message]

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=tip-0982adc746736a313dac9cb8cc936ca51ca3741a@git.kernel.org \
    --to=tipbot@zytor.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=bhe@redhat.com \
    --cc=hpa@zytor.com \
    --cc=j-nomura@ce.jp.nec.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tip-commits@vger.kernel.org \
    --cc=matt@codeblueprint.co.uk \
    --cc=mingo@kernel.org \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=thgarnie@google.com \
    --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: link
Be 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.