Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1682192 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from  KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-06 10:40 +0200
    Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Chao Fan <fanc.fnst@cn.fujitsu.com> - 2017-07-06 11:20 +0200
      Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-06 11:30 +0200
        Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Chao Fan <fanc.fnst@cn.fujitsu.com> - 2017-07-06 11:40 +0200
    Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Baoquan He <bhe@redhat.com> - 2017-07-06 11:20 +0200
      Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-06 11:40 +0200
    Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Chao Fan <fanc.fnst@cn.fujitsu.com> - 2017-07-06 12:10 +0200
      Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Chao Fan <fanc.fnst@cn.fujitsu.com> - 2017-07-06 12:30 +0200
    Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Matt Fleming <matt@codeblueprint.co.uk> - 2017-07-06 17:00 +0200
      Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Baoquan He <bhe@redhat.com> - 2017-07-07 05:10 +0200
        Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-07 08:20 +0200
          Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Matt Fleming <matt@codeblueprint.co.uk> - 2017-07-07 13:00 +0200
            Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-10 07:50 +0200
              [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-10 08:00 +0200
              [PATCH v3 1/2] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-10 08:00 +0200
        Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Matt Fleming <matt@codeblueprint.co.uk> - 2017-07-07 13:00 +0200
          Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Baoquan He <bhe@redhat.com> - 2017-07-09 12:50 +0200
            Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}  from KASLR's choice Baoquan He <bhe@redhat.com> - 2017-07-09 16:30 +0200
    [PATCH v2 2/2] x86/efi: clean up dead code around efi_reserve_boot_services() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-07 09:30 +0200
    [PATCH v2 1/2] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-07 09:30 +0200

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

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@ah.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@ah.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(&region, minimum, image_size);
+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
 			efi_mirror_found = true;
+	}
+
+	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(&region, 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

[toc] | [next] | [standalone]

#1682232 — 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@ah.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@ah.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(&region, minimum, image_size);
>+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> 			efi_mirror_found = true;

Hi Horiguchi-san,

If efi_mirror_found is changed to be true, we won't need to walk other
entries, so I think:
		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
 			efi_mirror_found = true;
			break;
		}
will be enough to show that mirror regions exist. And will walk
less entries. How do you think about this?

Another question: what's the benifit of putting this part of
"efi_mirror_found = true" to a independent cycle.

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(&region, 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
>
>
>

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jul 06, 2017 at 05:13:32PM +0800, Chao Fan wrote:
> 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@ah.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@ah.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(&region, minimum, image_size);
> >+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> > 			efi_mirror_found = true;
> 
> Hi Horiguchi-san,
> 
> If efi_mirror_found is changed to be true, we won't need to walk other
> entries, so I think:
> 		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>  			efi_mirror_found = true;
> 			break;
> 		}
> will be enough to show that mirror regions exist. And will walk
> less entries. How do you think about this?

Thank you for the review, Chao.
And you're right, I'll add break here.

# I'll post revised one tomorrow waiting for more comments.

> Another question: what's the benifit of putting this part of
> "efi_mirror_found = true" to a independent cycle.

We can't easily cancel process_mem_region(), so if we process a few normal
regions like EFI_CONVENTIONAL_MEMORY and then find a EFI_MEMORY_MORE_RELIABLE
region, that's a bit troublesome.
So I decided to first check whether EFI_MEMORY_MORE_RELIABLE region exists or not.

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jul 06, 2017 at 09:22:38AM +0000, Naoya Horiguchi wrote:
>On Thu, Jul 06, 2017 at 05:13:32PM +0800, Chao Fan wrote:
>> 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@ah.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@ah.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(&region, minimum, image_size);
>> >+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>> > 			efi_mirror_found = true;
>> 
>> Hi Horiguchi-san,
>> 
>> If efi_mirror_found is changed to be true, we won't need to walk other
>> entries, so I think:
>> 		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>>  			efi_mirror_found = true;
>> 			break;
>> 		}
>> will be enough to show that mirror regions exist. And will walk
>> less entries. How do you think about this?
>
>Thank you for the review, Chao.
>And you're right, I'll add break here.
>
># I'll post revised one tomorrow waiting for more comments.
>
>> Another question: what's the benifit of putting this part of
>> "efi_mirror_found = true" to a independent cycle.
>
>We can't easily cancel process_mem_region(), so if we process a few normal
>regions like EFI_CONVENTIONAL_MEMORY and then find a EFI_MEMORY_MORE_RELIABLE
>region, that's a bit troublesome.
>So I decided to first check whether EFI_MEMORY_MORE_RELIABLE region exists or not.

OK, I got it. Thanks for your explanation.

Thanks,
Chao Fan
>
>Thanks,
>Naoya Horiguchi
>
>

[toc] | [prev] | [next] | [standalone]

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

Hi Naoya Horiguchi,

Thanks for making this!

On 07/06/17 at 08:31am, 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?

This is interesting. So you are suggesting that we should try to avoid
those EFI_BOOT_SERVICES_{CODE|DATA} efi regions as long as efi map
regions are available, meanwhile try to locate kernel inside mirrored
regions if existed. I do know the efi work around, so it seems reasonable
to me, I can add it when repost. Or you can post after mine has been
merged.

A little adjustment, please see the inline comment.
> 
> [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@ah.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@ah.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(&region, minimum, image_size);
> +		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>  			efi_mirror_found = true;

Here, we should define a local variable of bool type to mark if mirrored
region is found.

> +	}
> +
> +	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(&region, minimum, image_size);

And can define a new global variable like efi_processed here.
		efi_processed = true;


And yes, I missed  this snippet of code to break loop if slot_area has
been full, this saves time.

> +		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
> 

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jul 06, 2017 at 05:18:09PM +0800, Baoquan He wrote:
> Hi Naoya Horiguchi,
> 
> Thanks for making this!
> 
> On 07/06/17 at 08:31am, 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?
> 
> This is interesting. So you are suggesting that we should try to avoid
> those EFI_BOOT_SERVICES_{CODE|DATA} efi regions as long as efi map
> regions are available, meanwhile try to locate kernel inside mirrored
> regions if existed. I do know the efi work around, so it seems reasonable
> to me, I can add it when repost. Or you can post after mine has been
> merged.

Thank you for the positive response, Baoquan.

> 
> A little adjustment, please see the inline comment.
> > 
> > [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@ah.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@ah.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(&region, minimum, image_size);
> > +		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> >  			efi_mirror_found = true;
> 
> Here, we should define a local variable of bool type to mark if mirrored
> region is found.

OK. efi_mirror_found need not be a global variable any longer.

> > +	}
> > +
> > +	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(&region, minimum, image_size);
> 
> And can define a new global variable like efi_processed here.
> 		efi_processed = true;

I intented that this is represented by the return value of process_efi_entry(),
but if you have more readable/clearer options, I'm fine with that.

> 
> 
> And yes, I missed  this snippet of code to break loop if slot_area has
> been full, this saves time.

No problem, feel free to move this snippet to your 2/2 patch.

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

#1682300 — 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@ah.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@ah.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(&region, 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(&region, 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
>
>
>

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jul 06, 2017 at 06:04:46PM +0800, Chao Fan wrote:
>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@ah.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@ah.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(&region, 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.

Sorry for disturbing, it's my misunderstanding.
My suggestion is useless.

Since efi entries have been walked, we can use them directly,
no need to walk e820 entries again.

The logic of your codes is good.
Sorry again.

Thanks,
Chao Fan

>
>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(&region, 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
>>
>>
>>

[toc] | [prev] | [next] | [standalone]

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

On Thu, 06 Jul, at 08:31:07AM, 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.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.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(&region, minimum, image_size);
> +		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>  			efi_mirror_found = true;
> +	}
> +
> +	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;

Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?

You can't re-use EFI_LOADER_* regions because the kaslr code is run so
early in boot that you've no idea if data the kernel will need is in
those EFI_LOADER_* regions.

For example, we pass struct setup_data objects inside of
EFI_LOADER_DATA regions.

[toc] | [prev] | [next] | [standalone]

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

On 07/06/17 at 03:57pm, Matt Fleming wrote:
> On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > +	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;
> 
> Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> 
> You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> early in boot that you've no idea if data the kernel will need is in
> those EFI_LOADER_* regions.
> 
> For example, we pass struct setup_data objects inside of
> EFI_LOADER_DATA regions.

It doesn't matter because we have tried to avoid those memory setup_data
resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
discard the whole regions while setup_data could occupy small part of
them.

[toc] | [prev] | [next] | [standalone]

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

On Fri, Jul 07, 2017 at 11:07:59AM +0800, Baoquan He wrote:
> On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > +	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;
> > 
> > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > 
> > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > early in boot that you've no idea if data the kernel will need is in
> > those EFI_LOADER_* regions.
> > 
> > For example, we pass struct setup_data objects inside of
> > EFI_LOADER_DATA regions.
> 
> It doesn't matter because we have tried to avoid those memory setup_data
> resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> discard the whole regions while setup_data could occupy small part of
> them.

Hi Matt, Baoquan,

I added these three checks to accept any regions corresponding to
E820_TYPE_RAM except EFI_BOOT_SERVICES_*, just thinking of that it's minimum
surprising.  Baoquan gave a good justification on that, so I'll leave it
as-is in next version.

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

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

On Fri, 07 Jul, at 06:11:24AM, Naoya Horiguchi wrote:
> On Fri, Jul 07, 2017 at 11:07:59AM +0800, Baoquan He wrote:
> > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > +	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;
> > > 
> > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > > 
> > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > early in boot that you've no idea if data the kernel will need is in
> > > those EFI_LOADER_* regions.
> > > 
> > > For example, we pass struct setup_data objects inside of
> > > EFI_LOADER_DATA regions.
> > 
> > It doesn't matter because we have tried to avoid those memory setup_data
> > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > discard the whole regions while setup_data could occupy small part of
> > them.
> 
> Hi Matt, Baoquan,
> 
> I added these three checks to accept any regions corresponding to
> E820_TYPE_RAM except EFI_BOOT_SERVICES_*, just thinking of that it's minimum
> surprising.  Baoquan gave a good justification on that, so I'll leave it
> as-is in next version.

I disagree. The least surprising option would be to use the region
type that everyone (boot loader, kernel, firmware) agrees is free:
EFI_CONVENTIONAL_MEMORY.

[toc] | [prev] | [next] | [standalone]

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

On Fri, Jul 07, 2017 at 11:58:14AM +0100, Matt Fleming wrote:
> On Fri, 07 Jul, at 06:11:24AM, Naoya Horiguchi wrote:
> > On Fri, Jul 07, 2017 at 11:07:59AM +0800, Baoquan He wrote:
> > > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > > +	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;
> > > > 
> > > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > > > 
> > > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > > early in boot that you've no idea if data the kernel will need is in
> > > > those EFI_LOADER_* regions.
> > > > 
> > > > For example, we pass struct setup_data objects inside of
> > > > EFI_LOADER_DATA regions.
> > > 
> > > It doesn't matter because we have tried to avoid those memory setup_data
> > > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > > discard the whole regions while setup_data could occupy small part of
> > > them.
> > 
> > Hi Matt, Baoquan,
> > 
> > I added these three checks to accept any regions corresponding to
> > E820_TYPE_RAM except EFI_BOOT_SERVICES_*, just thinking of that it's minimum
> > surprising.  Baoquan gave a good justification on that, so I'll leave it
> > as-is in next version.
> 
> I disagree. The least surprising option would be to use the region
> type that everyone (boot loader, kernel, firmware) agrees is free:
> EFI_CONVENTIONAL_MEMORY.

OK, I will do it.

[toc] | [prev] | [next] | [standalone]

#1683967 — [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()

EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
so we can clean up the check in efi_reserve_boot_services().

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 arch/x86/platform/efi/quirks.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git next-20170705/arch/x86/platform/efi/quirks.c next-20170705_patched/arch/x86/platform/efi/quirks.c
index 8a99a2e..191f6f7 100644
--- next-20170705/arch/x86/platform/efi/quirks.c
+++ next-20170705_patched/arch/x86/platform/efi/quirks.c
@@ -292,27 +292,6 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 	efi_memmap_install(new_phys, num_entries);
 }
 
-/*
- * Helper function for efi_reserve_boot_services() to figure out if we
- * can free regions in efi_free_boot_services().
- *
- * Use this function to ensure we do not free regions owned by somebody
- * else. We must only reserve (and then free) regions:
- *
- * - Not within any part of the kernel
- * - Not the BIOS reserved area (E820_TYPE_RESERVED, E820_TYPE_NVS, etc)
- */
-static bool can_free_region(u64 start, u64 size)
-{
-	if (start + size > __pa_symbol(_text) && start <= __pa_symbol(_end))
-		return false;
-
-	if (!e820__mapped_all(start, start+size, E820_TYPE_RAM))
-		return false;
-
-	return true;
-}
-
 void __init efi_reserve_boot_services(void)
 {
 	efi_memory_desc_t *md;
@@ -350,7 +329,7 @@ void __init efi_reserve_boot_services(void)
 			 * one else cares about it. We own it and can
 			 * free it later.
 			 */
-			if (can_free_region(start, size))
+			if (e820__mapped_all(start, start+size, E820_TYPE_RAM))
 				continue;
 		}
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1683970 — [PATCH v3 1/2] 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@ah.jp.nec.com>
---
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 | 44 +++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git next-20170705/arch/x86/boot/compressed/kaslr.c next-20170705_patched/arch/x86/boot/compressed/kaslr.c
index 94f08fd..44778e9 100644
--- next-20170705/arch/x86/boot/compressed/kaslr.c
+++ next-20170705_patched/arch/x86/boot/compressed/kaslr.c
@@ -560,10 +560,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;
@@ -572,18 +570,18 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
 	char *signature;
 	u32 nr_desc;
 	int i;
-
+	bool efi_mirror_found;
 
 	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
@@ -594,12 +592,35 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_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(&region, minimum, image_size);
 			efi_mirror_found = true;
+			break;
 		}
 	}
+
+	for (i = 0; i < nr_desc; i++) {
+		md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
+
+		/*
+		 * 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.
+		 */
+		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;
 }
 
 static void process_e820_entry(unsigned long minimum, unsigned long image_size)
@@ -637,8 +658,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.0

[toc] | [prev] | [next] | [standalone]

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

On Fri, 07 Jul, at 11:07:59AM, Baoquan He wrote:
> On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > +	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;
> > 
> > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > 
> > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > early in boot that you've no idea if data the kernel will need is in
> > those EFI_LOADER_* regions.
> > 
> > For example, we pass struct setup_data objects inside of
> > EFI_LOADER_DATA regions.
> 
> It doesn't matter because we have tried to avoid those memory setup_data
> resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> discard the whole regions while setup_data could occupy small part of
> them.

What about the GDT that we allocate in the x86 EFI boot stub as
EFI_LOADER_DATA? Are there functions to avoid that too?

What about any future uses we add? Who's going to remember to patch
the kaslr code which now duplicates some of the EFI memory map logic?

All of these problems can avoided if you just stick with
EFI_CONVENTIONAL_MEMORY.

Honestly, how much memory do we expect to waste if we ignore
EFI_LOADER_* regions?

Also, the fact that you're referencing EFI-specific boot quirks in the
kaslr code should be a massive red flag that you're playing with the
innards of the EFI subsystem.

[toc] | [prev] | [next] | [standalone]

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

On 07/07/17 at 11:56am, Matt Fleming wrote:
> On Fri, 07 Jul, at 11:07:59AM, Baoquan He wrote:
> > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > +	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;
> > > 
> > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > > 
> > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > early in boot that you've no idea if data the kernel will need is in
> > > those EFI_LOADER_* regions.
> > > 
> > > For example, we pass struct setup_data objects inside of
> > > EFI_LOADER_DATA regions.
> > 
> > It doesn't matter because we have tried to avoid those memory setup_data
> > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > discard the whole regions while setup_data could occupy small part of
> > them.
> 
> What about the GDT that we allocate in the x86 EFI boot stub as
> EFI_LOADER_DATA? Are there functions to avoid that too?

This is a very good question. For the current e820 processing, we don't
avoid GDT allocated in x86 EFI STUB because we have no information about
GDT in EFI STUB. You can see setup_e820() in boot/compressed/eboot.c
grabs EFI_LOADER_* regions as E820_TYPE_RAM. Now the GDT which is built
in EFI STUB code will live till kernel is ready to build its onw gdt(in
kernel/head_64.S). After that it become useless and can be reclaimed for
reusing. I believe Naoya must have read boot/compressed/eboot.c and
found setup_e820() code, then added EFI_LOADER_* for kaslr usage.

Previously I thought e820 should take the precedence to be processed
on uefi system because continuous efi memory regions will be merged into
e820 regions. Using e820 can avoid those small efi regions or the left
part of efi regions being discarded directly when they are smaller than
kernel image size. Now considering this GDT in efi stub issue, we have to
try efi regions first on uefi system. Otherwise it may cause problem that
kernel could be decompressed onto the GDT tables of efi stub.

In fact, I am wondering if we can reuse the gdt table which is built
before entering into long mode in boot/compressed/head_64.S, but not
allocate memory for GDT in efi stub. The thing is 32bit system doesn't
have this gdt table in boot compressing stage since it has one before
entering into protection mode. Just personal thought.
> 
> What about any future uses we add? Who's going to remember to patch
> the kaslr code which now duplicates some of the EFI memory map logic?
> 
> All of these problems can avoided if you just stick with
> EFI_CONVENTIONAL_MEMORY.

Below is the dmesg with 'efi=debug' adding on my ovmf uefi kvm guest.
Since uefi could do a lot of thing when loading OS, E.g loading into any
kind of storage driver application, firmware usually reserve a chunk of
memory of hunderes of Mega Bytes. Like this one:

******
[  +0.000000] efi: mem12: [Loader Data        | ||WB|WT|WC|UC] range=[0x000000005c8eb000-0x000000007bfbdfff] (502MB)
******

We can't say it's a big deal, but 500MB is also not so trival that we
can easily ignore it without any consideration. Just uefi spec doesn't
define the limitation of Loader memory and Conventional memory and if
Conventional memory has to be present. With my understanding, there won't
be any problem if only Loader memory exists.

Further more, kaslr is not a precise searching job, no specific address
has to be positioned. Even it's OK that the physical address
randomization failed to find a new address randomly. So it's fine to me
that we don't take EFI_LOADER_* memory into consideration for kaslr. BUT
we need make this clear that why not, and if we can do anyting to make
it better.

[  +0.000000] efi:  SMBIOS=0x7fed5000  ACPI=0x7ff03000  ACPI 2.0=0x7ff03014  MEMATTR=0x7ea50218
[  +0.000000] efi: mem00: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000000000-0x0000000000000fff] (0MB)
[  +0.000000] efi: mem01: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000001000-0x0000000000001fff] (0MB)
[  +0.000000] efi: mem02: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000002000-0x000000000009ffff] (0MB)
[  +0.000000] efi: mem03: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000100000-0x0000000000805fff] (7MB)
[  +0.000000] efi: mem04: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000806000-0x0000000000806fff] (0MB)
[  +0.000000] efi: mem05: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000807000-0x000000000081ffff] (0MB)
[  +0.000000] efi: mem06: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000820000-0x00000000012fffff] (10MB)
[  +0.000000] efi: mem07: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000001300000-0x0000000001ffffff] (13MB)
[  +0.000000] efi: mem08: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000002000000-0x0000000003614fff] (22MB)
[  +0.000000] efi: mem09: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000003615000-0x000000003d6b3fff] (928MB)
[  +0.000000] efi: mem10: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000003d6b4000-0x000000003fffffff] (41MB)
[  +0.000000] efi: mem11: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000040000000-0x000000005c8eafff] (456MB)
[  +0.000000] efi: mem12: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000005c8eb000-0x000000007bfbdfff] (502MB)
[  +0.000000] efi: mem13: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007bfbe000-0x000000007bfddfff] (0MB)
[  +0.000000] efi: mem14: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007bfde000-0x000000007e6effff] (39MB)
[  +0.000000] efi: mem15: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e6f0000-0x000000007e7e3fff] (0MB)
[  +0.000000] efi: mem16: [Loader Code        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e7e4000-0x000000007e90afff] (1MB)
[  +0.000000] efi: mem17: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e90b000-0x000000007e914fff] (0MB)
[  +0.000000] efi: mem18: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e915000-0x000000007ea46fff] (1MB)
[  +0.000000] efi: mem19: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ea47000-0x000000007eb8dfff] (1MB)
[  +0.000000] efi: mem20: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007eb8e000-0x000000007edd0fff] (2MB)
[  +0.000000] efi: mem21: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007edd1000-0x000000007edd5fff] (0MB)
[  +0.000000] efi: mem22: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007edd6000-0x000000007edddfff] (0MB)
[  +0.000000] efi: mem23: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007edde000-0x000000007ede2fff] (0MB)
[  +0.000000] efi: mem24: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ede3000-0x000000007ede8fff] (0MB)
[  +0.000000] efi: mem25: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ede9000-0x000000007ee12fff] (0MB)
[  +0.000000] efi: mem26: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ee13000-0x000000007ee23fff] (0MB)
[  +0.000000] efi: mem27: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ee24000-0x000000007f46dfff] (6MB)
[  +0.000000] efi: mem28: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007f46e000-0x000000007f477fff] (0MB)
[  +0.000000] efi: mem29: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007f478000-0x000000007fd23fff] (8MB)
[  +0.000000] efi: mem30: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fd24000-0x000000007fd24fff] (0MB)
[  +0.000000] efi: mem31: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fd25000-0x000000007fea3fff] (1MB)
[  +0.000000] efi: mem32: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fea4000-0x000000007fed3fff] (0MB)
[  +0.000000] efi: mem33: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fed4000-0x000000007fef7fff] (0MB)
[  +0.000000] efi: mem34: [Reserved           |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fef8000-0x000000007fefbfff] (0MB)
[  +0.000000] efi: mem35: [ACPI Reclaim Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fefc000-0x000000007ff03fff] (0MB)
[  +0.000000] efi: mem36: [ACPI Memory NVS    |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff04000-0x000000007ff07fff] (0MB)
[  +0.000000] efi: mem37: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff08000-0x000000007ff6afff] (0MB)
[  +0.000000] efi: mem38: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff6b000-0x000000007ff95fff] (0MB)
[  +0.000000] efi: mem39: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff96000-0x000000007ffa6fff] (0MB)
[  +0.000000] efi: mem40: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ffa7000-0x000000007ffcffff] (0MB)
[  +0.000000] efi: mem41: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ffd0000-0x000000007ffeffff] (0MB)
[  +0.000000] efi: mem42: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fff0000-0x000000007fffffff] (0MB)
[  +0.000000] efi: mem43: [Runtime Data       |RUN|  |  |  |  |  |  ||  |  |  |UC] range=[0x00000000ffe00000-0x00000000ffffffff] (2MB)

> 
> Honestly, how much memory do we expect to waste if we ignore
> EFI_LOADER_* regions?
> 
> Also, the fact that you're referencing EFI-specific boot quirks in the
> kaslr code should be a massive red flag that you're playing with the
> innards of the EFI subsystem.

Questions has been answered in above words.

[toc] | [prev] | [next] | [standalone]

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

On 07/09/17 at 06:44pm, Baoquan He wrote:
> On 07/07/17 at 11:56am, Matt Fleming wrote:
> > On Fri, 07 Jul, at 11:07:59AM, Baoquan He wrote:
> > > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > > +	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;
> > > > 
> > > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > > > 
> > > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > > early in boot that you've no idea if data the kernel will need is in
> > > > those EFI_LOADER_* regions.
> > > > 
> > > > For example, we pass struct setup_data objects inside of
> > > > EFI_LOADER_DATA regions.
> > > 
> > > It doesn't matter because we have tried to avoid those memory setup_data
> > > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > > discard the whole regions while setup_data could occupy small part of
> > > them.
> > 
> > What about the GDT that we allocate in the x86 EFI boot stub as
> > EFI_LOADER_DATA? Are there functions to avoid that too?
> 
> This is a very good question. For the current e820 processing, we don't
> avoid GDT allocated in x86 EFI STUB because we have no information about
> GDT in EFI STUB. You can see setup_e820() in boot/compressed/eboot.c

I was wrong here, we can know gdt information by sgdt. Surely GDT is
just an example, as Matt mentioned, there could be other stuffs we need
avoid, or future change of uefi.

> grabs EFI_LOADER_* regions as E820_TYPE_RAM. Now the GDT which is built
> in EFI STUB code will live till kernel is ready to build its onw gdt(in
> kernel/head_64.S). After that it become useless and can be reclaimed for
> reusing. I believe Naoya must have read boot/compressed/eboot.c and
> found setup_e820() code, then added EFI_LOADER_* for kaslr usage.
> 
> Previously I thought e820 should take the precedence to be processed
> on uefi system because continuous efi memory regions will be merged into
> e820 regions. Using e820 can avoid those small efi regions or the left
> part of efi regions being discarded directly when they are smaller than
> kernel image size. Now considering this GDT in efi stub issue, we have to
> try efi regions first on uefi system. Otherwise it may cause problem that
> kernel could be decompressed onto the GDT tables of efi stub.
> 
> In fact, I am wondering if we can reuse the gdt table which is built
> before entering into long mode in boot/compressed/head_64.S, but not
> allocate memory for GDT in efi stub. The thing is 32bit system doesn't
> have this gdt table in boot compressing stage since it has one before
> entering into protection mode. Just personal thought.
> > 
> > What about any future uses we add? Who's going to remember to patch
> > the kaslr code which now duplicates some of the EFI memory map logic?
> > 
> > All of these problems can avoided if you just stick with
> > EFI_CONVENTIONAL_MEMORY.
> 
> Below is the dmesg with 'efi=debug' adding on my ovmf uefi kvm guest.
> Since uefi could do a lot of thing when loading OS, E.g loading into any
> kind of storage driver application, firmware usually reserve a chunk of
> memory of hunderes of Mega Bytes. Like this one:
> 
> ******
> [  +0.000000] efi: mem12: [Loader Data        | ||WB|WT|WC|UC] range=[0x000000005c8eb000-0x000000007bfbdfff] (502MB)
> ******
> 
> We can't say it's a big deal, but 500MB is also not so trival that we
> can easily ignore it without any consideration. Just uefi spec doesn't
> define the limitation of Loader memory and Conventional memory and if
> Conventional memory has to be present. With my understanding, there won't
> be any problem if only Loader memory exists.
> 
> Further more, kaslr is not a precise searching job, no specific address
> has to be positioned. Even it's OK that the physical address
> randomization failed to find a new address randomly. So it's fine to me
> that we don't take EFI_LOADER_* memory into consideration for kaslr. BUT
> we need make this clear that why not, and if we can do anyting to make
> it better.
> 
> [  +0.000000] efi:  SMBIOS=0x7fed5000  ACPI=0x7ff03000  ACPI 2.0=0x7ff03014  MEMATTR=0x7ea50218
> [  +0.000000] efi: mem00: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000000000-0x0000000000000fff] (0MB)
> [  +0.000000] efi: mem01: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000001000-0x0000000000001fff] (0MB)
> [  +0.000000] efi: mem02: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000002000-0x000000000009ffff] (0MB)
> [  +0.000000] efi: mem03: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000100000-0x0000000000805fff] (7MB)
> [  +0.000000] efi: mem04: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000806000-0x0000000000806fff] (0MB)
> [  +0.000000] efi: mem05: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000807000-0x000000000081ffff] (0MB)
> [  +0.000000] efi: mem06: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000000820000-0x00000000012fffff] (10MB)
> [  +0.000000] efi: mem07: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000001300000-0x0000000001ffffff] (13MB)
> [  +0.000000] efi: mem08: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000002000000-0x0000000003614fff] (22MB)
> [  +0.000000] efi: mem09: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000003615000-0x000000003d6b3fff] (928MB)
> [  +0.000000] efi: mem10: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000003d6b4000-0x000000003fffffff] (41MB)
> [  +0.000000] efi: mem11: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x0000000040000000-0x000000005c8eafff] (456MB)
> [  +0.000000] efi: mem12: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000005c8eb000-0x000000007bfbdfff] (502MB)
> [  +0.000000] efi: mem13: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007bfbe000-0x000000007bfddfff] (0MB)
> [  +0.000000] efi: mem14: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007bfde000-0x000000007e6effff] (39MB)
> [  +0.000000] efi: mem15: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e6f0000-0x000000007e7e3fff] (0MB)
> [  +0.000000] efi: mem16: [Loader Code        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e7e4000-0x000000007e90afff] (1MB)
> [  +0.000000] efi: mem17: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e90b000-0x000000007e914fff] (0MB)
> [  +0.000000] efi: mem18: [Loader Data        |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007e915000-0x000000007ea46fff] (1MB)
> [  +0.000000] efi: mem19: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ea47000-0x000000007eb8dfff] (1MB)
> [  +0.000000] efi: mem20: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007eb8e000-0x000000007edd0fff] (2MB)
> [  +0.000000] efi: mem21: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007edd1000-0x000000007edd5fff] (0MB)
> [  +0.000000] efi: mem22: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007edd6000-0x000000007edddfff] (0MB)
> [  +0.000000] efi: mem23: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007edde000-0x000000007ede2fff] (0MB)
> [  +0.000000] efi: mem24: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ede3000-0x000000007ede8fff] (0MB)
> [  +0.000000] efi: mem25: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ede9000-0x000000007ee12fff] (0MB)
> [  +0.000000] efi: mem26: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ee13000-0x000000007ee23fff] (0MB)
> [  +0.000000] efi: mem27: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ee24000-0x000000007f46dfff] (6MB)
> [  +0.000000] efi: mem28: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007f46e000-0x000000007f477fff] (0MB)
> [  +0.000000] efi: mem29: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007f478000-0x000000007fd23fff] (8MB)
> [  +0.000000] efi: mem30: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fd24000-0x000000007fd24fff] (0MB)
> [  +0.000000] efi: mem31: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fd25000-0x000000007fea3fff] (1MB)
> [  +0.000000] efi: mem32: [Runtime Code       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fea4000-0x000000007fed3fff] (0MB)
> [  +0.000000] efi: mem33: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fed4000-0x000000007fef7fff] (0MB)
> [  +0.000000] efi: mem34: [Reserved           |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fef8000-0x000000007fefbfff] (0MB)
> [  +0.000000] efi: mem35: [ACPI Reclaim Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fefc000-0x000000007ff03fff] (0MB)
> [  +0.000000] efi: mem36: [ACPI Memory NVS    |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff04000-0x000000007ff07fff] (0MB)
> [  +0.000000] efi: mem37: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff08000-0x000000007ff6afff] (0MB)
> [  +0.000000] efi: mem38: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff6b000-0x000000007ff95fff] (0MB)
> [  +0.000000] efi: mem39: [Boot Data          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ff96000-0x000000007ffa6fff] (0MB)
> [  +0.000000] efi: mem40: [Boot Code          |   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ffa7000-0x000000007ffcffff] (0MB)
> [  +0.000000] efi: mem41: [Runtime Data       |RUN|  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007ffd0000-0x000000007ffeffff] (0MB)
> [  +0.000000] efi: mem42: [Conventional Memory|   |  |  |  |  |  |  |  |WB|WT|WC|UC] range=[0x000000007fff0000-0x000000007fffffff] (0MB)
> [  +0.000000] efi: mem43: [Runtime Data       |RUN|  |  |  |  |  |  ||  |  |  |UC] range=[0x00000000ffe00000-0x00000000ffffffff] (2MB)
> 
> > 
> > Honestly, how much memory do we expect to waste if we ignore
> > EFI_LOADER_* regions?
> > 
> > Also, the fact that you're referencing EFI-specific boot quirks in the
> > kaslr code should be a massive red flag that you're playing with the
> > innards of the EFI subsystem.
> 
> Questions has been answered in above words.

[toc] | [prev] | [next] | [standalone]

#1682979 — [PATCH v2 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()

EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
so we can clean up the check in efi_reserve_boot_services().

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 arch/x86/platform/efi/quirks.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git next-20170705/arch/x86/platform/efi/quirks.c next-20170705_patched/arch/x86/platform/efi/quirks.c
index 8a99a2e..191f6f7 100644
--- next-20170705/arch/x86/platform/efi/quirks.c
+++ next-20170705_patched/arch/x86/platform/efi/quirks.c
@@ -292,27 +292,6 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 	efi_memmap_install(new_phys, num_entries);
 }
 
-/*
- * Helper function for efi_reserve_boot_services() to figure out if we
- * can free regions in efi_free_boot_services().
- *
- * Use this function to ensure we do not free regions owned by somebody
- * else. We must only reserve (and then free) regions:
- *
- * - Not within any part of the kernel
- * - Not the BIOS reserved area (E820_TYPE_RESERVED, E820_TYPE_NVS, etc)
- */
-static bool can_free_region(u64 start, u64 size)
-{
-	if (start + size > __pa_symbol(_text) && start <= __pa_symbol(_end))
-		return false;
-
-	if (!e820__mapped_all(start, start+size, E820_TYPE_RAM))
-		return false;
-
-	return true;
-}
-
 void __init efi_reserve_boot_services(void)
 {
 	efi_memory_desc_t *md;
@@ -350,7 +329,7 @@ void __init efi_reserve_boot_services(void)
 			 * one else cares about it. We own it and can
 			 * free it later.
 			 */
-			if (can_free_region(start, size))
+			if (e820__mapped_all(start, start+size, E820_TYPE_RAM))
 				continue;
 		}
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1682985 — [PATCH v2 1/2] 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@ah.jp.nec.com>
---
v1 -> v2:
- switch efi_mirror_found to local variable
- insert break when EFI_MEMORY_MORE_RELIABLE found
---
 arch/x86/boot/compressed/kaslr.c | 46 +++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git next-20170705/arch/x86/boot/compressed/kaslr.c next-20170705_patched/arch/x86/boot/compressed/kaslr.c
index 94f08fd..29f5c4e 100644
--- next-20170705/arch/x86/boot/compressed/kaslr.c
+++ next-20170705_patched/arch/x86/boot/compressed/kaslr.c
@@ -560,10 +560,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;
@@ -572,18 +570,18 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
 	char *signature;
 	u32 nr_desc;
 	int i;
-
+	bool efi_mirror_found;
 
 	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
@@ -594,12 +592,37 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_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(&region, minimum, image_size);
 			efi_mirror_found = true;
+			break;
 		}
 	}
+
+	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(&region, 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 +660,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.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web