[PATCH v4] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice

[PATCH v4] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice

[Naoya Horiguchi]

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. Furthermore,
we choose kernel address only from EFI_CONVENTIONAL_MEMORY because it's
the only memory type we know to be free.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
v3 -> v4:
- update comment and patch description to mention why only
  EFI_CONVENTIONAL_MEMORY is chosen.
- use efi_early_memdesc_ptr()
- I decided not to post cleanup patches (patch 2/2 in previous series)
  because it's not necessary to fix the issue.

v2 -> v3:
- skip EFI_LOADER_CODE and EFI_LOADER_DATA in region scan

v1 -> v2:
- switch efi_mirror_found to local variable
- insert break when EFI_MEMORY_MORE_RELIABLE found
---
 arch/x86/boot/compressed/kaslr.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git tip/x86/boot/arch/x86/boot/compressed/kaslr.c tip/x86/boot_patched/arch/x86/boot/compressed/kaslr.c
index 7de23bb..ba5e9e5 100644
--- tip/x86/boot/arch/x86/boot/compressed/kaslr.c
+++ tip/x86/boot_patched/arch/x86/boot/compressed/kaslr.c
@@ -597,19 +597,36 @@ 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);
+
+		/*
+		 * According to spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
+		 * available for kernel image, but we don't include them for
+		 * the workaround for buggy firmware.
+		 * 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
-- 
2.7.0

Re: [PATCH v4] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice

[Baoquan He]

Hi Naoya,

Thanks for this fix. I saw NEC had reported a bug to rhel previously,
and the bug truly will corrupt OS, it can be fixed by this patch.

This patch looks good to me, just a small concern, please see below
inline comment.

On 08/24/17 at 07:33pm, Naoya Horiguchi wrote:
> 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. Furthermore,
> we choose kernel address only from EFI_CONVENTIONAL_MEMORY because it's
> the only memory type we know to be free.

Here, I think it's better to present why EFI_CONVENTIONAL_MEMORY is the
only memory type we should choose. EFI_BOOT_SERVICES_xxx has been clear
to us why it's not good. It might be worth to saying something about
EFI_LOADER_xxx why it's not ok to choose. Maybe one sentence to mention
it and take pgd as exampel as Matt ever said.

Thanks
Baoquan

> v3 -> v4:
> - update comment and patch description to mention why only
>   EFI_CONVENTIONAL_MEMORY is chosen.
> - use efi_early_memdesc_ptr()
> - I decided not to post cleanup patches (patch 2/2 in previous series)
>   because it's not necessary to fix the issue.
> 
> v2 -> v3:
> - skip EFI_LOADER_CODE and EFI_LOADER_DATA in region scan
> 
> v1 -> v2:
> - switch efi_mirror_found to local variable
> - insert break when EFI_MEMORY_MORE_RELIABLE found
> ---
>  arch/x86/boot/compressed/kaslr.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git tip/x86/boot/arch/x86/boot/compressed/kaslr.c tip/x86/boot_patched/arch/x86/boot/compressed/kaslr.c
> index 7de23bb..ba5e9e5 100644
> --- tip/x86/boot/arch/x86/boot/compressed/kaslr.c
> +++ tip/x86/boot_patched/arch/x86/boot/compressed/kaslr.c
> @@ -597,19 +597,36 @@ 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);
> +
> +		/*
> +		 * According to spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
> +		 * available for kernel image, but we don't include them for
> +		 * the workaround for buggy firmware.
> +		 * 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
> -- 
> 2.7.0
> 

[PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice

[Naoya Horiguchi]

On Mon, Aug 28, 2017 at 02:59:51PM +0800, Baoquan He wrote:
> Hi Naoya,
> 
> Thanks for this fix. I saw NEC had reported a bug to rhel previously,
> and the bug truly will corrupt OS, it can be fixed by this patch.
> 
> This patch looks good to me, just a small concern, please see below
> inline comment.
> 
> On 08/24/17 at 07:33pm, Naoya Horiguchi wrote:
> > 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. Furthermore,
> > we choose kernel address only from EFI_CONVENTIONAL_MEMORY because it's
> > the only memory type we know to be free.
> 
> Here, I think it's better to present why EFI_CONVENTIONAL_MEMORY is the
> only memory type we should choose. EFI_BOOT_SERVICES_xxx has been clear
> to us why it's not good. It might be worth to saying something about
> EFI_LOADER_xxx why it's not ok to choose. Maybe one sentence to mention
> it and take pgd as exampel as Matt ever said.

According to EFI spec, EFI_LOADER_* regions can be used for some purposes
after ExitBootServices(), which is the reason for us to exclude them.

  Table 26. Memory Type Usage after ExitBootServices()

  ...
  EfiLoaderCode               The UEFI OS Loader and/or OS may use this memory as they see fit.
                              Note: the UEFI OS loader that called
                              EFI_BOOT_SERVICES.ExitBootServices() is utilizing one or
                              more EfiLoaderCode ranges.
  EfiLoaderData               The Loader and/or OS may use this memory as they see fit. Note: the
                              OS loader that called ExitBootServices() is utilizing one or more
                              EfiLoaderData ranges.
  ...
  EfiBootServicesCode         Memory available for general use.
  EfiBootServicesData         Memory available for general use.
  EfiConventionalMemory       Memory available for general use.

Here is an updated patch.

Thanks,
Naoya Horiguchi
---
>From 93182d1d8c2ce11232f7686c01dc16ee96e062c4 Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Mon, 28 Aug 2017 16:30:59 +0900
Subject: [PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_*
 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. 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 free.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
v4 -> v5:
- addressed why EFI_LOADER_* is excluded.
- changed patch subject.

v3 -> v4:
- update comment and patch description to mention why only
  EFI_CONVENTIONAL_MEMORY is chosen.
- use efi_early_memdesc_ptr()
- I decided not to post cleanup patches (patch 2/2 in previous series)
  because it's not necessary to fix the issue.

v2 -> v3:
- skip EFI_LOADER_CODE and EFI_LOADER_DATA in region scan

v1 -> v2:
- switch efi_mirror_found to local variable
- insert break when EFI_MEMORY_MORE_RELIABLE found
---
 arch/x86/boot/compressed/kaslr.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 7de23bb279ce..ba5e9e5aaa89 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -597,19 +597,36 @@ 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);
+
+		/*
+		 * According to spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
+		 * available for kernel image, but we don't include them for
+		 * the workaround for buggy firmware.
+		 * 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
-- 
2.7.4

Re: [PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice

[Baoquan He]

On 08/28/17 at 07:44am, Naoya Horiguchi wrote:
> From 93182d1d8c2ce11232f7686c01dc16ee96e062c4 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Date: Mon, 28 Aug 2017 16:30:59 +0900
> Subject: [PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_*
>  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. 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 free.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks, looks good to me. Ack it.

Acked-by: Baoquan He <bhe@redhat.com>

> ---
> v4 -> v5:
> - addressed why EFI_LOADER_* is excluded.
> - changed patch subject.
> 
> v3 -> v4:
> - update comment and patch description to mention why only
>   EFI_CONVENTIONAL_MEMORY is chosen.
> - use efi_early_memdesc_ptr()
> - I decided not to post cleanup patches (patch 2/2 in previous series)
>   because it's not necessary to fix the issue.
> 
> v2 -> v3:
> - skip EFI_LOADER_CODE and EFI_LOADER_DATA in region scan
> 
> v1 -> v2:
> - switch efi_mirror_found to local variable
> - insert break when EFI_MEMORY_MORE_RELIABLE found
> ---
>  arch/x86/boot/compressed/kaslr.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 7de23bb279ce..ba5e9e5aaa89 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -597,19 +597,36 @@ 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);
> +
> +		/*
> +		 * According to spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
> +		 * available for kernel image, but we don't include them for
> +		 * the workaround for buggy firmware.
> +		 * 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
> -- 
> 2.7.4
> 

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

[tip-bot for Naoya Horiguchi]

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