| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170706100446.GC26868@localhost.localdomain>
Date: Thu, 6 Jul 2017 18:04:46 +0800
From: Chao Fan <fanc.fnst@...fujitsu.com>
To: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
CC: Baoquan He <bhe@...hat.com>, Kees Cook <keescook@...omium.org>,
LKML <linux-kernel@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
"izumi.taku@...fujitsu.com" <izumi.taku@...fujitsu.com>,
Thomas Garnier <thgarnie@...gle.com>,
Matt Fleming <matt@...eblueprint.co.uk>,
Junichi Nomura <j-nomura@...jp.nec.com>
Subject: Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}
from KASLR's choice
On Thu, Jul 06, 2017 at 08:31:07AM +0000, Naoya Horiguchi wrote:
>Hi Baoquan, everyone,
>
>I'm also interested in KASLR/EFI related issue (but not the same issue
>with yours, so I separated the thread.)
>
>This patch is based on Baoquan's recent patches[1], adding more code
>on the new function process_efi_entry().
>If it's OK, could you queue this onto your tree/series?
>
>[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
> https://lkml.org/lkml/2017/7/5/98
>
>Thanks,
>Naoya Horiguchi
>---
>From: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
>Date: Thu, 6 Jul 2017 16:40:52 +0900
>Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> KASLR's choice
>
>KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>UEFI spec, all memory regions marked as EfiBootServicesCode and
>EfiBootServicesData are available for free memory after the first call
>of ExitBootServices(). So such regions should be usable for kernel on
>spec basis.
>
>In x86, however, we have some workaround for broken 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 == EFI_BOOT_SERVICES_CODE ||
> md->type == 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.
>
>Signed-off-by: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
>---
> arch/x86/boot/compressed/kaslr.c | 41 +++++++++++++++++++++++++++++++---------
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 94f08fd375ae..f43fed0441a6 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> /* Marks if efi mirror regions have been found and handled. */
> static bool efi_mirror_found;
>
>-static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>+/* Returns true if we really enter efi memmap walk, otherwise returns false. */
>+static bool process_efi_entry(unsigned long minimum, unsigned long image_size)
> {
> struct efi_info *e = &boot_params->efi_info;
> struct mem_vector region;
>@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
> signature = (char *)&boot_params->efi_info.efi_loader_signature;
> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>- return;
>+ return false;
>
> #ifdef CONFIG_X86_32
> /* Can't handle data above 4GB at this time */
> if (e->efi_memmap_hi) {
> warn("Memory map is above 4GB, EFI should be disabled.\n");
>- return;
>+ return false;
> }
> pmap = e->efi_memmap;
> #else
>@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> for (i = 0; i < nr_desc; i++) {
> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>- region.start = md->phys_addr;
>- region.size = md->num_pages << EFI_PAGE_SHIFT;
>- process_mem_region(®ion, minimum, image_size);
>+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> efi_mirror_found = true;
>+ }
Hi Horiguchi-san,
Sorry for one more suggestion,
How about add:
if (!efi_mirror_found)
return false;
at this place, between the two cycles.
Because if there are no mirror regions found, I think we can return
directly and go to walk the e820 entries.
I don't know whether my understanding is right.
Thanks,
Chao Fan
>+
>+ for (i = 0; i < nr_desc; i++) {
>+ md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>+
>+ /*
>+ * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
>+ * services regions could be accessed after ExitBootServices()
>+ * due to the workaround for buggy firmware.
>+ */
>+ if (!(md->type == EFI_LOADER_CODE ||
>+ md->type == EFI_LOADER_DATA ||
>+ 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(®ion, minimum, image_size);
>+ if (slot_area_index == MAX_SLOT_AREA) {
>+ debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>+ break;
> }
> }
>+ return true;
> }
>
> static void process_e820_entry(unsigned long minimum, unsigned long image_size)
>@@ -637,8 +661,7 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
> minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>
> #ifdef CONFIG_EFI
>- process_efi_entry(minimum, image_size);
>- if (efi_mirror_found)
>+ if (process_efi_entry(minimum, image_size))
> return slots_fetch_random();
> #endif
>
>--
>2.7.4
>
>
>
Powered by blists - more mailing lists