All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v9 0/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions
@ 2017-08-14 14:54 Baoquan He
  2017-08-14 14:54 ` [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor Baoquan He
  2017-08-14 14:54 ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
  0 siblings, 2 replies; 14+ messages in thread
From: Baoquan He @ 2017-08-14 14:54 UTC (permalink / raw)
  To: mingo, matt
  Cc: linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86,
	Baoquan He

Patch 1/2 is added to add efi_early_memdesc_ptr helper to wrap the open
code which gets the start of efi memmap descriptor and also explain why
it need be done like that, Ingo suggested it.

And also replace several places of the open code with efi_early_memdesc_ptr
helper.

And also use efi_early_memdesc_ptr in process_efi_entries() which handle efi
mirror issue during KASLR.


Change:
v8->v9:
    Matt suggested that an 'early' should be included in the name of
    the helper to make its name as efi_early_memdesc_ptr. That can help
    denote the helper should only be used in early bootup stage, after
    that for_each_efi_memory_*() API is suggested.

v7->v8:
    Add efi_memdesc_ptr helper to wrap the open code which gets the
    start of map descriptor according to Ingo's suggestion.

v6->v7:
  Ingo pointed out several incorrect line break issues and unclear
  description of patch log. Correct them and rewrite patch log.

  And also rewrite the EFI warning message that if EFI memmap is above
  4G in 32bit system since 32bit system can not handle data above 4G at
  kernel decompression stage. This is suggested by Ingo too.

v5->v6:
  Code style issue fix according to Kees's comment.

  This is based on tip/x86/boot, patch 1,2,3/4 in v5 post has
  been put into tip/x86/boot now.

Baoquan He (2):
  efi: Introduce efi_early_memdesc_ptr to get pointer to memmap
    descriptor
  x86/boot/KASLR: Restrict kernel to be randomized in mirror regions

 arch/x86/boot/compressed/eboot.c               |  2 +-
 arch/x86/boot/compressed/kaslr.c               | 68 +++++++++++++++++++++++++-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  4 +-
 include/linux/efi.h                            | 21 ++++++++
 4 files changed, 90 insertions(+), 5 deletions(-)

-- 
2.5.5

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
  2017-08-14 14:54 [PATCH v9 0/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
@ 2017-08-14 14:54 ` Baoquan He
  2017-08-16 11:37     ` Matt Fleming
  2017-08-16 13:46   ` [PATCH v10 " Baoquan He
  2017-08-14 14:54 ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
  1 sibling, 2 replies; 14+ messages in thread
From: Baoquan He @ 2017-08-14 14:54 UTC (permalink / raw)
  To: mingo, matt
  Cc: linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86,
	Baoquan He

The existing map iteration helper for_each_efi_memory_desc_in_map can
only be used after OS initializes EFI to fill data of struct efi_memory_map.
Before that we also need iterate map descriptors which are stored in several
intermediate structures, like struct efi_boot_memmap for arch independent
usage and struct efi_info for x86 arch only.

Introduce efi_early_memdesc_ptr to get pointer to a map descriptor, and
replace several places of open code with it.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/eboot.c               |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
 include/linux/efi.h                            | 21 +++++++++++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c3e869eaef0c..e007887a33b0 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
 		m |= (u64)efi->efi_memmap_hi << 32;
 #endif
 
-		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
+		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
 		switch (d->type) {
 		case EFI_RESERVED_TYPE:
 		case EFI_RUNTIME_SERVICES_CODE:
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index b0184360efc6..50a9cab5a834 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -205,7 +205,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 		unsigned long m = (unsigned long)map;
 		u64 start, end;
 
-		desc = (efi_memory_desc_t *)(m + (i * desc_size));
+		desc = efi_early_memdesc_ptr(m, desc_size, i);
 		if (desc->type != EFI_CONVENTIONAL_MEMORY)
 			continue;
 
@@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 		unsigned long m = (unsigned long)map;
 		u64 start, end;
 
-		desc = (efi_memory_desc_t *)(m + (i * desc_size));
+		desc = efi_early_memdesc_ptr(m, desc_size, i);
 
 		if (desc->type != EFI_CONVENTIONAL_MEMORY)
 			continue;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8269bcb8ccf7..9783d9e4a4b2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1020,6 +1020,27 @@ extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn);
 
+/*
+ * efi_early_memdesc_ptr - get the n-th efi memmap descriptor
+ * @map: the start of efi memmap
+ * @desc_size: the size of space for each efi memmap descriptor
+ * @n: the index of efi memmap descriptor
+ *
+ * EFI boot service provides function GetMemoryMap() to get a copy of the
+ * current memory map which is an array of memory descriptors, each of
+ * which describes a contiguous block of memory. And also get the size of
+ * map, and the size of each descriptor, etc. Note that per section 6.2 of
+ * UEFI Spec 2.6 Errata A, the returned size of each descriptor might not
+ * be equal to sizeof(efi_memory_memdesc_t) since efi_memory_memdesc_t may
+ * be extended in the future in response to hardware innovation. Thus OS
+ * MUST use the returned size of descriptor to find the start of each
+ * efi_memory_memdesc_t in the memory map array. This should only be used
+ * during bootup since for_each_efi_memory_desc_xxx is suggested after OS
+ * initializes EFI to fill data of struct efi_memory_map.
+ */
+#define efi_early_memdesc_ptr(map, desc_size, n)			\
+	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
+
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions
  2017-08-14 14:54 [PATCH v9 0/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
  2017-08-14 14:54 ` [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor Baoquan He
@ 2017-08-14 14:54 ` Baoquan He
  2017-08-17 10:21   ` [tip:x86/boot] x86/boot/KASLR: Prefer mirrored memory regions for the kernel physical address tip-bot for Baoquan He
  2017-08-17 13:04   ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
  1 sibling, 2 replies; 14+ messages in thread
From: Baoquan He @ 2017-08-14 14:54 UTC (permalink / raw)
  To: mingo, matt
  Cc: linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86,
	Baoquan He

Currently KASLR will parse all e820 entries of RAM type and add all
candidate position into slots array. Then we will choose one slot
randomly as the new position which kernel will be decompressed into
and run at.

On system with EFI enabled, e820 memory regions are coming from EFI
memory regions by combining adjacent regions. While these EFI memory
regions have more attributes to mark their different use. Mirror
attribute is such kind. The physical memory region whose descriptors
in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are
mirrored. The address range mirroring feature of kernel arranges such
mirror region into normal zone and other region into movable zone. And
with mirroring feature enabled, the code and date of kernel can only be
located in more reliable mirror region. However, the current KASLR code
doesn't check EFI memory entries, and could choose new position in
non-mirrored region. This will break the functionality of the address
range mirroring feature.

So if EFI is detected, iterate EFI memory map and pick the mirror region
to process for adding candidate of randomization slot. If EFI is disabled
or no mirror region found, still process e820 memory map.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 99c7194f7ea6..7de23bb279ce 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -37,7 +37,9 @@
 #include <linux/uts.h>
 #include <linux/utsname.h>
 #include <linux/ctype.h>
+#include <linux/efi.h>
 #include <generated/utsrelease.h>
+#include <asm/efi.h>
 
 /* Macros used by the included decompressor code below. */
 #define STATIC
@@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+#ifdef CONFIG_EFI
+/*
+ * Returns true if mirror region found (and must have been processed
+ * for slots adding)
+ */
+static bool
+process_efi_entries(unsigned long minimum, unsigned long image_size)
+{
+	struct efi_info *e = &boot_params->efi_info;
+	bool efi_mirror_found = false;
+	struct mem_vector region;
+	efi_memory_desc_t *md;
+	unsigned long pmap;
+	char *signature;
+	u32 nr_desc;
+	int i;
+
+	signature = (char *)&e->efi_loader_signature;
+	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
+	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
+		return false;
+
+#ifdef CONFIG_X86_32
+	/* Can't handle data above 4GB at this time */
+	if (e->efi_memmap_hi) {
+		warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n");
+		return false;
+	}
+	pmap =  e->efi_memmap;
+#else
+	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
+#endif
+
+	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
+	for (i = 0; i < nr_desc; i++) {
+		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
+			region.start = md->phys_addr;
+			region.size = md->num_pages << EFI_PAGE_SHIFT;
+			process_mem_region(&region, minimum, image_size);
+			efi_mirror_found = true;
+
+			if (slot_area_index == MAX_SLOT_AREA) {
+				debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+				break;
+			}
+		}
+	}
+
+	return efi_mirror_found;
+}
+#else
+static inline bool
+process_efi_entries(unsigned long minimum, unsigned long image_size)
+{
+	return false;
+}
+#endif
+
 static void process_e820_entries(unsigned long minimum,
 				 unsigned long image_size)
 {
@@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
 {
 	/* Check if we had too many memmaps. */
 	if (memmap_too_large) {
-		debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
+		debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
 		return 0;
 	}
 
 	/* Make sure minimum is aligned. */
 	minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
 
+	if (process_efi_entries(minimum, image_size))
+		return slots_fetch_random();
+
 	process_e820_entries(minimum, image_size);
 	return slots_fetch_random();
 }
@@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
 	 */
 	min_addr = min(*output, 512UL << 20);
 
-	/* Walk e820 and find a random address. */
+	/* Walk available memory entries to find a random address. */
 	random_addr = find_random_phys_addr(min_addr, output_size);
 	if (!random_addr) {
 		warn("Physical KASLR disabled: no suitable memory region!");
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
@ 2017-08-16 11:37     ` Matt Fleming
  0 siblings, 0 replies; 14+ messages in thread
From: Matt Fleming @ 2017-08-16 11:37 UTC (permalink / raw)
  To: Baoquan He
  Cc: mingo, linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86

On Mon, 14 Aug, at 10:54:23PM, Baoquan He wrote:
> The existing map iteration helper for_each_efi_memory_desc_in_map can
> only be used after OS initializes EFI to fill data of struct efi_memory_map.

Should this say "EFI subsystem"? The firmware doesn't care about the
kernel's internal data structures.

> Before that we also need iterate map descriptors which are stored in several
> intermediate structures, like struct efi_boot_memmap for arch independent
> usage and struct efi_info for x86 arch only.
> 
> Introduce efi_early_memdesc_ptr to get pointer to a map descriptor, and
> replace several places of open code with it.
> 
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/eboot.c               |  2 +-
>  drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
>  include/linux/efi.h                            | 21 +++++++++++++++++++++
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index c3e869eaef0c..e007887a33b0 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
>  		m |= (u64)efi->efi_memmap_hi << 32;
>  #endif
>  
> -		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
> +		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
>  		switch (d->type) {
>  		case EFI_RESERVED_TYPE:
>  		case EFI_RUNTIME_SERVICES_CODE:
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index b0184360efc6..50a9cab5a834 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -205,7 +205,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
>  		unsigned long m = (unsigned long)map;
>  		u64 start, end;
>  
> -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> +		desc = efi_early_memdesc_ptr(m, desc_size, i);
>  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
>  			continue;
>  
> @@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
>  		unsigned long m = (unsigned long)map;
>  		u64 start, end;
>  
> -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> +		desc = efi_early_memdesc_ptr(m, desc_size, i);
>  
>  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
>  			continue;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8269bcb8ccf7..9783d9e4a4b2 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1020,6 +1020,27 @@ extern int efi_memattr_init(void);
>  extern int efi_memattr_apply_permissions(struct mm_struct *mm,
>  					 efi_memattr_perm_setter fn);
>  
> +/*
> + * efi_early_memdesc_ptr - get the n-th efi memmap descriptor
> + * @map: the start of efi memmap
> + * @desc_size: the size of space for each efi memmap descriptor
> + * @n: the index of efi memmap descriptor
> + *
> + * EFI boot service provides function GetMemoryMap() to get a copy of the
> + * current memory map which is an array of memory descriptors, each of
> + * which describes a contiguous block of memory. And also get the size of
> + * map, and the size of each descriptor, etc. Note that per section 6.2 of
> + * UEFI Spec 2.6 Errata A, the returned size of each descriptor might not
> + * be equal to sizeof(efi_memory_memdesc_t) since efi_memory_memdesc_t may
> + * be extended in the future in response to hardware innovation. Thus OS
> + * MUST use the returned size of descriptor to find the start of each
> + * efi_memory_memdesc_t in the memory map array. This should only be used
> + * during bootup since for_each_efi_memory_desc_xxx is suggested after OS
> + * initializes EFI to fill data of struct efi_memory_map.
> + */

Again, please use "EFI subsystem" here.

> +#define efi_early_memdesc_ptr(map, desc_size, n)			\
> +	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
> +
>  /* Iterate through an efi_memory_map */
>  #define for_each_efi_memory_desc_in_map(m, md)				   \
>  	for ((md) = (m)->map;						   \

Otherwise, this looks OK to me.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
@ 2017-08-16 11:37     ` Matt Fleming
  0 siblings, 0 replies; 14+ messages in thread
From: Matt Fleming @ 2017-08-16 11:37 UTC (permalink / raw)
  To: Baoquan He
  Cc: mingo-DgEjT+Ai2ygdnm+yROfE0A,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	keescook-F7+t8E8rja9g9hUCZPvPmw, tglx-hfZtesqFncYOwBW4kG4KsQ,
	hpa-YMNOUZJC4hwAvxtiuMwx3w, izumi.taku-+CUm20s59erQFUHtdCDX3A,
	fanc.fnst-BthXqXjhjHXQFUHtdCDX3A,
	thgarnie-hpIqsD4AKlfQT0dZR+AlfA,
	n-horiguchi-PaJj6Psr51x8UrSeD/g0lQ,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	linux-efi-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A

On Mon, 14 Aug, at 10:54:23PM, Baoquan He wrote:
> The existing map iteration helper for_each_efi_memory_desc_in_map can
> only be used after OS initializes EFI to fill data of struct efi_memory_map.

Should this say "EFI subsystem"? The firmware doesn't care about the
kernel's internal data structures.

> Before that we also need iterate map descriptors which are stored in several
> intermediate structures, like struct efi_boot_memmap for arch independent
> usage and struct efi_info for x86 arch only.
> 
> Introduce efi_early_memdesc_ptr to get pointer to a map descriptor, and
> replace several places of open code with it.
> 
> Suggested-by: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/boot/compressed/eboot.c               |  2 +-
>  drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
>  include/linux/efi.h                            | 21 +++++++++++++++++++++
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index c3e869eaef0c..e007887a33b0 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
>  		m |= (u64)efi->efi_memmap_hi << 32;
>  #endif
>  
> -		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
> +		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
>  		switch (d->type) {
>  		case EFI_RESERVED_TYPE:
>  		case EFI_RUNTIME_SERVICES_CODE:
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index b0184360efc6..50a9cab5a834 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -205,7 +205,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
>  		unsigned long m = (unsigned long)map;
>  		u64 start, end;
>  
> -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> +		desc = efi_early_memdesc_ptr(m, desc_size, i);
>  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
>  			continue;
>  
> @@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
>  		unsigned long m = (unsigned long)map;
>  		u64 start, end;
>  
> -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> +		desc = efi_early_memdesc_ptr(m, desc_size, i);
>  
>  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
>  			continue;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8269bcb8ccf7..9783d9e4a4b2 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1020,6 +1020,27 @@ extern int efi_memattr_init(void);
>  extern int efi_memattr_apply_permissions(struct mm_struct *mm,
>  					 efi_memattr_perm_setter fn);
>  
> +/*
> + * efi_early_memdesc_ptr - get the n-th efi memmap descriptor
> + * @map: the start of efi memmap
> + * @desc_size: the size of space for each efi memmap descriptor
> + * @n: the index of efi memmap descriptor
> + *
> + * EFI boot service provides function GetMemoryMap() to get a copy of the
> + * current memory map which is an array of memory descriptors, each of
> + * which describes a contiguous block of memory. And also get the size of
> + * map, and the size of each descriptor, etc. Note that per section 6.2 of
> + * UEFI Spec 2.6 Errata A, the returned size of each descriptor might not
> + * be equal to sizeof(efi_memory_memdesc_t) since efi_memory_memdesc_t may
> + * be extended in the future in response to hardware innovation. Thus OS
> + * MUST use the returned size of descriptor to find the start of each
> + * efi_memory_memdesc_t in the memory map array. This should only be used
> + * during bootup since for_each_efi_memory_desc_xxx is suggested after OS
> + * initializes EFI to fill data of struct efi_memory_map.
> + */

Again, please use "EFI subsystem" here.

> +#define efi_early_memdesc_ptr(map, desc_size, n)			\
> +	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
> +
>  /* Iterate through an efi_memory_map */
>  #define for_each_efi_memory_desc_in_map(m, md)				   \
>  	for ((md) = (m)->map;						   \

Otherwise, this looks OK to me.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
@ 2017-08-16 13:18       ` Baoquan He
  0 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2017-08-16 13:18 UTC (permalink / raw)
  To: Matt Fleming
  Cc: mingo, linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86

On 08/16/17 at 12:37pm, Matt Fleming wrote:
> On Mon, 14 Aug, at 10:54:23PM, Baoquan He wrote:
> > The existing map iteration helper for_each_efi_memory_desc_in_map can
> > only be used after OS initializes EFI to fill data of struct efi_memory_map.
> 
> Should this say "EFI subsystem"? The firmware doesn't care about the
> kernel's internal data structures.

Sounds reasonable. Let me update and maybe repost to this thread
directly. Thanks!

> 
> > Before that we also need iterate map descriptors which are stored in several
> > intermediate structures, like struct efi_boot_memmap for arch independent
> > usage and struct efi_info for x86 arch only.
> > 
> > Introduce efi_early_memdesc_ptr to get pointer to a map descriptor, and
> > replace several places of open code with it.
> > 
> > Suggested-by: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/eboot.c               |  2 +-
> >  drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
> >  include/linux/efi.h                            | 21 +++++++++++++++++++++
> >  3 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index c3e869eaef0c..e007887a33b0 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
> >  		m |= (u64)efi->efi_memmap_hi << 32;
> >  #endif
> >  
> > -		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
> > +		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
> >  		switch (d->type) {
> >  		case EFI_RESERVED_TYPE:
> >  		case EFI_RUNTIME_SERVICES_CODE:
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > index b0184360efc6..50a9cab5a834 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -205,7 +205,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
> >  		unsigned long m = (unsigned long)map;
> >  		u64 start, end;
> >  
> > -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> > +		desc = efi_early_memdesc_ptr(m, desc_size, i);
> >  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
> >  			continue;
> >  
> > @@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
> >  		unsigned long m = (unsigned long)map;
> >  		u64 start, end;
> >  
> > -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> > +		desc = efi_early_memdesc_ptr(m, desc_size, i);
> >  
> >  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
> >  			continue;
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 8269bcb8ccf7..9783d9e4a4b2 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1020,6 +1020,27 @@ extern int efi_memattr_init(void);
> >  extern int efi_memattr_apply_permissions(struct mm_struct *mm,
> >  					 efi_memattr_perm_setter fn);
> >  
> > +/*
> > + * efi_early_memdesc_ptr - get the n-th efi memmap descriptor
> > + * @map: the start of efi memmap
> > + * @desc_size: the size of space for each efi memmap descriptor
> > + * @n: the index of efi memmap descriptor
> > + *
> > + * EFI boot service provides function GetMemoryMap() to get a copy of the
> > + * current memory map which is an array of memory descriptors, each of
> > + * which describes a contiguous block of memory. And also get the size of
> > + * map, and the size of each descriptor, etc. Note that per section 6.2 of
> > + * UEFI Spec 2.6 Errata A, the returned size of each descriptor might not
> > + * be equal to sizeof(efi_memory_memdesc_t) since efi_memory_memdesc_t may
> > + * be extended in the future in response to hardware innovation. Thus OS
> > + * MUST use the returned size of descriptor to find the start of each
> > + * efi_memory_memdesc_t in the memory map array. This should only be used
> > + * during bootup since for_each_efi_memory_desc_xxx is suggested after OS
> > + * initializes EFI to fill data of struct efi_memory_map.
> > + */
> 
> Again, please use "EFI subsystem" here.

Sure, will do.
> 
> > +#define efi_early_memdesc_ptr(map, desc_size, n)			\
> > +	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
> > +
> >  /* Iterate through an efi_memory_map */
> >  #define for_each_efi_memory_desc_in_map(m, md)				   \
> >  	for ((md) = (m)->map;						   \
> 
> Otherwise, this looks OK to me.

Thanks for reviewing!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
@ 2017-08-16 13:18       ` Baoquan He
  0 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2017-08-16 13:18 UTC (permalink / raw)
  To: Matt Fleming
  Cc: mingo-DgEjT+Ai2ygdnm+yROfE0A,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	keescook-F7+t8E8rja9g9hUCZPvPmw, tglx-hfZtesqFncYOwBW4kG4KsQ,
	hpa-YMNOUZJC4hwAvxtiuMwx3w, izumi.taku-+CUm20s59erQFUHtdCDX3A,
	fanc.fnst-BthXqXjhjHXQFUHtdCDX3A,
	thgarnie-hpIqsD4AKlfQT0dZR+AlfA,
	n-horiguchi-PaJj6Psr51x8UrSeD/g0lQ,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	linux-efi-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A

On 08/16/17 at 12:37pm, Matt Fleming wrote:
> On Mon, 14 Aug, at 10:54:23PM, Baoquan He wrote:
> > The existing map iteration helper for_each_efi_memory_desc_in_map can
> > only be used after OS initializes EFI to fill data of struct efi_memory_map.
> 
> Should this say "EFI subsystem"? The firmware doesn't care about the
> kernel's internal data structures.

Sounds reasonable. Let me update and maybe repost to this thread
directly. Thanks!

> 
> > Before that we also need iterate map descriptors which are stored in several
> > intermediate structures, like struct efi_boot_memmap for arch independent
> > usage and struct efi_info for x86 arch only.
> > 
> > Introduce efi_early_memdesc_ptr to get pointer to a map descriptor, and
> > replace several places of open code with it.
> > 
> > Suggested-by: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/boot/compressed/eboot.c               |  2 +-
> >  drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
> >  include/linux/efi.h                            | 21 +++++++++++++++++++++
> >  3 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index c3e869eaef0c..e007887a33b0 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
> >  		m |= (u64)efi->efi_memmap_hi << 32;
> >  #endif
> >  
> > -		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
> > +		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
> >  		switch (d->type) {
> >  		case EFI_RESERVED_TYPE:
> >  		case EFI_RUNTIME_SERVICES_CODE:
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > index b0184360efc6..50a9cab5a834 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -205,7 +205,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
> >  		unsigned long m = (unsigned long)map;
> >  		u64 start, end;
> >  
> > -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> > +		desc = efi_early_memdesc_ptr(m, desc_size, i);
> >  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
> >  			continue;
> >  
> > @@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
> >  		unsigned long m = (unsigned long)map;
> >  		u64 start, end;
> >  
> > -		desc = (efi_memory_desc_t *)(m + (i * desc_size));
> > +		desc = efi_early_memdesc_ptr(m, desc_size, i);
> >  
> >  		if (desc->type != EFI_CONVENTIONAL_MEMORY)
> >  			continue;
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 8269bcb8ccf7..9783d9e4a4b2 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1020,6 +1020,27 @@ extern int efi_memattr_init(void);
> >  extern int efi_memattr_apply_permissions(struct mm_struct *mm,
> >  					 efi_memattr_perm_setter fn);
> >  
> > +/*
> > + * efi_early_memdesc_ptr - get the n-th efi memmap descriptor
> > + * @map: the start of efi memmap
> > + * @desc_size: the size of space for each efi memmap descriptor
> > + * @n: the index of efi memmap descriptor
> > + *
> > + * EFI boot service provides function GetMemoryMap() to get a copy of the
> > + * current memory map which is an array of memory descriptors, each of
> > + * which describes a contiguous block of memory. And also get the size of
> > + * map, and the size of each descriptor, etc. Note that per section 6.2 of
> > + * UEFI Spec 2.6 Errata A, the returned size of each descriptor might not
> > + * be equal to sizeof(efi_memory_memdesc_t) since efi_memory_memdesc_t may
> > + * be extended in the future in response to hardware innovation. Thus OS
> > + * MUST use the returned size of descriptor to find the start of each
> > + * efi_memory_memdesc_t in the memory map array. This should only be used
> > + * during bootup since for_each_efi_memory_desc_xxx is suggested after OS
> > + * initializes EFI to fill data of struct efi_memory_map.
> > + */
> 
> Again, please use "EFI subsystem" here.

Sure, will do.
> 
> > +#define efi_early_memdesc_ptr(map, desc_size, n)			\
> > +	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
> > +
> >  /* Iterate through an efi_memory_map */
> >  #define for_each_efi_memory_desc_in_map(m, md)				   \
> >  	for ((md) = (m)->map;						   \
> 
> Otherwise, this looks OK to me.

Thanks for reviewing!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v10 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
  2017-08-14 14:54 ` [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor Baoquan He
  2017-08-16 11:37     ` Matt Fleming
@ 2017-08-16 13:46   ` Baoquan He
  2017-08-17 10:20     ` [tip:x86/boot] " tip-bot for Baoquan He
  1 sibling, 1 reply; 14+ messages in thread
From: Baoquan He @ 2017-08-16 13:46 UTC (permalink / raw)
  To: mingo, matt
  Cc: linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86

The existing map iteration helper for_each_efi_memory_desc_in_map can
only be used after OS initializes EFI subsystem to fill data of struct
efi_memory_map. Before that we also need iterate map descriptors which
are stored in several intermediate structures, like struct efi_boot_memmap
for arch independent usage and struct efi_info for x86 arch only.

Introduce efi_early_memdesc_ptr to get pointer to a map descriptor, and
replace several places of open code with it.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v9->v10:
  Use the 'EFI subsystem' instead of EFI since EFI usually means
  firmware. Here we are talking about EFI subsystem inside kernel.

 arch/x86/boot/compressed/eboot.c               |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
 include/linux/efi.h                            | 21 +++++++++++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c3e869eaef0c..e007887a33b0 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
 		m |= (u64)efi->efi_memmap_hi << 32;
 #endif
 
-		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
+		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
 		switch (d->type) {
 		case EFI_RESERVED_TYPE:
 		case EFI_RUNTIME_SERVICES_CODE:
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index b0184360efc6..50a9cab5a834 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -205,7 +205,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 		unsigned long m = (unsigned long)map;
 		u64 start, end;
 
-		desc = (efi_memory_desc_t *)(m + (i * desc_size));
+		desc = efi_early_memdesc_ptr(m, desc_size, i);
 		if (desc->type != EFI_CONVENTIONAL_MEMORY)
 			continue;
 
@@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 		unsigned long m = (unsigned long)map;
 		u64 start, end;
 
-		desc = (efi_memory_desc_t *)(m + (i * desc_size));
+		desc = efi_early_memdesc_ptr(m, desc_size, i);
 
 		if (desc->type != EFI_CONVENTIONAL_MEMORY)
 			continue;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8269bcb8ccf7..ec296bc96bc5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1020,6 +1020,27 @@ extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn);
 
+/*
+ * efi_early_memdesc_ptr - get the n-th efi memmap descriptor
+ * @map: the start of efi memmap
+ * @desc_size: the size of space for each efi memmap descriptor
+ * @n: the index of efi memmap descriptor
+ *
+ * EFI boot service provides function GetMemoryMap() to get a copy of the
+ * current memory map which is an array of memory descriptors, each of
+ * which describes a contiguous block of memory. And also get the size of
+ * map, and the size of each descriptor, etc. Note that per section 6.2 of
+ * UEFI Spec 2.6 Errata A, the returned size of each descriptor might not
+ * be equal to sizeof(efi_memory_memdesc_t) since efi_memory_memdesc_t may
+ * be extended in the future in response to hardware innovation. Thus OS
+ * MUST use the returned size of descriptor to find the start of each
+ * efi_memory_memdesc_t in the memory map array. This should only be used
+ * during bootup since for_each_efi_memory_desc_xxx is suggested after OS
+ * initializes EFI subsystem to fill data of struct efi_memory_map.
+ */
+#define efi_early_memdesc_ptr(map, desc_size, n)			\
+	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
+
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [tip:x86/boot] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor
  2017-08-16 13:46   ` [PATCH v10 " Baoquan He
@ 2017-08-17 10:20     ` tip-bot for Baoquan He
  0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Baoquan He @ 2017-08-17 10:20 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: hpa, mingo, matt, linux-kernel, tglx, bhe, torvalds, peterz

Commit-ID:  02e43c2dcd3b3cf7244f6dda65a07e8dacadaf8d
Gitweb:     http://git.kernel.org/tip/02e43c2dcd3b3cf7244f6dda65a07e8dacadaf8d
Author:     Baoquan He <bhe@redhat.com>
AuthorDate: Wed, 16 Aug 2017 21:46:51 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 17 Aug 2017 10:50:57 +0200

efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor

The existing map iteration helper for_each_efi_memory_desc_in_map can
only be used after the kernel initializes the EFI subsystem to set up
struct efi_memory_map.

Before that we also need iterate map descriptors which are stored in several
intermediate structures, like struct efi_boot_memmap for arch independent
usage and struct efi_info for x86 arch only.

Introduce efi_early_memdesc_ptr() to get pointer to a map descriptor, and
replace several places where that primitive is open coded.

Signed-off-by: Baoquan He <bhe@redhat.com>
[ Various improvements to the text. ]
Acked-by: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: ard.biesheuvel@linaro.org
Cc: fanc.fnst@cn.fujitsu.com
Cc: izumi.taku@jp.fujitsu.com
Cc: keescook@chromium.org
Cc: linux-efi@vger.kernel.org
Cc: n-horiguchi@ah.jp.nec.com
Cc: thgarnie@google.com
Link: http://lkml.kernel.org/r/20170816134651.GF21273@x1
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/eboot.c               |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  4 ++--
 include/linux/efi.h                            | 22 ++++++++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c3e869e..e007887 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -767,7 +767,7 @@ static efi_status_t setup_e820(struct boot_params *params,
 		m |= (u64)efi->efi_memmap_hi << 32;
 #endif
 
-		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
+		d = efi_early_memdesc_ptr(m, efi->efi_memdesc_size, i);
 		switch (d->type) {
 		case EFI_RESERVED_TYPE:
 		case EFI_RUNTIME_SERVICES_CODE:
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index b018436..50a9cab 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -205,7 +205,7 @@ again:
 		unsigned long m = (unsigned long)map;
 		u64 start, end;
 
-		desc = (efi_memory_desc_t *)(m + (i * desc_size));
+		desc = efi_early_memdesc_ptr(m, desc_size, i);
 		if (desc->type != EFI_CONVENTIONAL_MEMORY)
 			continue;
 
@@ -298,7 +298,7 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 		unsigned long m = (unsigned long)map;
 		u64 start, end;
 
-		desc = (efi_memory_desc_t *)(m + (i * desc_size));
+		desc = efi_early_memdesc_ptr(m, desc_size, i);
 
 		if (desc->type != EFI_CONVENTIONAL_MEMORY)
 			continue;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8269bcb..a686ca9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1020,6 +1020,28 @@ extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn);
 
+/*
+ * efi_early_memdesc_ptr - get the n-th EFI memmap descriptor
+ * @map: the start of efi memmap
+ * @desc_size: the size of space for each EFI memmap descriptor
+ * @n: the index of efi memmap descriptor
+ *
+ * EFI boot service provides the GetMemoryMap() function to get a copy of the
+ * current memory map which is an array of memory descriptors, each of
+ * which describes a contiguous block of memory. It also gets the size of the
+ * map, and the size of each descriptor, etc.
+ *
+ * Note that per section 6.2 of UEFI Spec 2.6 Errata A, the returned size of
+ * each descriptor might not be equal to sizeof(efi_memory_memdesc_t),
+ * since efi_memory_memdesc_t may be extended in the future. Thus the OS
+ * MUST use the returned size of the descriptor to find the start of each
+ * efi_memory_memdesc_t in the memory map array. This should only be used
+ * during bootup since for_each_efi_memory_desc_xxx() is available after the
+ * kernel initializes the EFI subsystem to set up struct efi_memory_map.
+ */
+#define efi_early_memdesc_ptr(map, desc_size, n)			\
+	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
+
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [tip:x86/boot] x86/boot/KASLR: Prefer mirrored memory regions for the kernel physical address
  2017-08-14 14:54 ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
@ 2017-08-17 10:21   ` tip-bot for Baoquan He
  2017-08-17 13:04   ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
  1 sibling, 0 replies; 14+ messages in thread
From: tip-bot for Baoquan He @ 2017-08-17 10:21 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: hpa, peterz, bhe, linux-kernel, torvalds, mingo, tglx

Commit-ID:  c05cd79750fbe5415cda896bb99350603cc995ed
Gitweb:     http://git.kernel.org/tip/c05cd79750fbe5415cda896bb99350603cc995ed
Author:     Baoquan He <bhe@redhat.com>
AuthorDate: Mon, 14 Aug 2017 22:54:24 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 17 Aug 2017 10:51:35 +0200

x86/boot/KASLR: Prefer mirrored memory regions for the kernel physical address

Currently KASLR will parse all e820 entries of RAM type and add all
candidate positions into the slots array. After that we choose one slot
randomly as the new position which the kernel will be decompressed into
and run at.

On systems with EFI enabled, e820 memory regions are coming from EFI
memory regions by combining adjacent regions.

These EFI memory regions have various attributes, and the "mirrored"
attribute is one of them. The physical memory region whose descriptors
in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are
mirrored. The address range mirroring feature of the kernel arranges such
mirrored regions into normal zones and other regions into movable zones.

With the mirroring feature enabled, the code and data of the kernel can only
be located in the more reliable mirrored regions. However, the current KASLR
code doesn't check EFI memory entries, and could choose a new kernel position
in non-mirrored regions. This will break the intended functionality of the
address range mirroring feature.

To fix this, if EFI is detected, iterate EFI memory map and pick the mirrored
region to process for adding candidate of randomization slot. If EFI is disabled
or no mirrored region found, still process the e820 memory map.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: ard.biesheuvel@linaro.org
Cc: fanc.fnst@cn.fujitsu.com
Cc: izumi.taku@jp.fujitsu.com
Cc: keescook@chromium.org
Cc: linux-efi@vger.kernel.org
Cc: matt@codeblueprint.co.uk
Cc: n-horiguchi@ah.jp.nec.com
Cc: thgarnie@google.com
Link: http://lkml.kernel.org/r/1502722464-20614-3-git-send-email-bhe@redhat.com
[ Rewrote most of the text. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 99c7194f..7de23bb 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -37,7 +37,9 @@
 #include <linux/uts.h>
 #include <linux/utsname.h>
 #include <linux/ctype.h>
+#include <linux/efi.h>
 #include <generated/utsrelease.h>
+#include <asm/efi.h>
 
 /* Macros used by the included decompressor code below. */
 #define STATIC
@@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+#ifdef CONFIG_EFI
+/*
+ * Returns true if mirror region found (and must have been processed
+ * for slots adding)
+ */
+static bool
+process_efi_entries(unsigned long minimum, unsigned long image_size)
+{
+	struct efi_info *e = &boot_params->efi_info;
+	bool efi_mirror_found = false;
+	struct mem_vector region;
+	efi_memory_desc_t *md;
+	unsigned long pmap;
+	char *signature;
+	u32 nr_desc;
+	int i;
+
+	signature = (char *)&e->efi_loader_signature;
+	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
+	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
+		return false;
+
+#ifdef CONFIG_X86_32
+	/* Can't handle data above 4GB at this time */
+	if (e->efi_memmap_hi) {
+		warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n");
+		return false;
+	}
+	pmap =  e->efi_memmap;
+#else
+	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
+#endif
+
+	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
+	for (i = 0; i < nr_desc; i++) {
+		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
+			region.start = md->phys_addr;
+			region.size = md->num_pages << EFI_PAGE_SHIFT;
+			process_mem_region(&region, minimum, image_size);
+			efi_mirror_found = true;
+
+			if (slot_area_index == MAX_SLOT_AREA) {
+				debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+				break;
+			}
+		}
+	}
+
+	return efi_mirror_found;
+}
+#else
+static inline bool
+process_efi_entries(unsigned long minimum, unsigned long image_size)
+{
+	return false;
+}
+#endif
+
 static void process_e820_entries(unsigned long minimum,
 				 unsigned long image_size)
 {
@@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
 {
 	/* Check if we had too many memmaps. */
 	if (memmap_too_large) {
-		debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
+		debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
 		return 0;
 	}
 
 	/* Make sure minimum is aligned. */
 	minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
 
+	if (process_efi_entries(minimum, image_size))
+		return slots_fetch_random();
+
 	process_e820_entries(minimum, image_size);
 	return slots_fetch_random();
 }
@@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
 	 */
 	min_addr = min(*output, 512UL << 20);
 
-	/* Walk e820 and find a random address. */
+	/* Walk available memory entries to find a random address. */
 	random_addr = find_random_phys_addr(min_addr, output_size);
 	if (!random_addr) {
 		warn("Physical KASLR disabled: no suitable memory region!");

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions
  2017-08-14 14:54 ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
  2017-08-17 10:21   ` [tip:x86/boot] x86/boot/KASLR: Prefer mirrored memory regions for the kernel physical address tip-bot for Baoquan He
@ 2017-08-17 13:04   ` Baoquan He
  2017-08-18 15:10       ` Ard Biesheuvel
  1 sibling, 1 reply; 14+ messages in thread
From: Baoquan He @ 2017-08-17 13:04 UTC (permalink / raw)
  To: mingo
  Cc: linux-kernel, keescook, tglx, hpa, izumi.taku, fanc.fnst,
	thgarnie, n-horiguchi, ard.biesheuvel, linux-efi, x86, matt

On 08/14/17 at 10:54pm, Baoquan He wrote:
> Currently KASLR will parse all e820 entries of RAM type and add all
> candidate position into slots array. Then we will choose one slot
> randomly as the new position which kernel will be decompressed into
> and run at.
> 
> On system with EFI enabled, e820 memory regions are coming from EFI
> memory regions by combining adjacent regions. While these EFI memory
> regions have more attributes to mark their different use. Mirror
> attribute is such kind. The physical memory region whose descriptors
> in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are
> mirrored. The address range mirroring feature of kernel arranges such
> mirror region into normal zone and other region into movable zone. And
> with mirroring feature enabled, the code and date of kernel can only be
> located in more reliable mirror region. However, the current KASLR code
> doesn't check EFI memory entries, and could choose new position in
> non-mirrored region. This will break the functionality of the address
> range mirroring feature.

Thanks a lot for helping improving the patch log, Ingo! Will pay more
attention to the description in words and paragraph partition of log.

> 
> So if EFI is detected, iterate EFI memory map and pick the mirror region
> to process for adding candidate of randomization slot. If EFI is disabled
> or no mirror region found, still process e820 memory map.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 99c7194f7ea6..7de23bb279ce 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -37,7 +37,9 @@
>  #include <linux/uts.h>
>  #include <linux/utsname.h>
>  #include <linux/ctype.h>
> +#include <linux/efi.h>
>  #include <generated/utsrelease.h>
> +#include <asm/efi.h>
>  
>  /* Macros used by the included decompressor code below. */
>  #define STATIC
> @@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
>  	}
>  }
>  
> +#ifdef CONFIG_EFI
> +/*
> + * Returns true if mirror region found (and must have been processed
> + * for slots adding)
> + */
> +static bool
> +process_efi_entries(unsigned long minimum, unsigned long image_size)
> +{
> +	struct efi_info *e = &boot_params->efi_info;
> +	bool efi_mirror_found = false;
> +	struct mem_vector region;
> +	efi_memory_desc_t *md;
> +	unsigned long pmap;
> +	char *signature;
> +	u32 nr_desc;
> +	int i;
> +
> +	signature = (char *)&e->efi_loader_signature;
> +	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> +	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> +		return false;
> +
> +#ifdef CONFIG_X86_32
> +	/* Can't handle data above 4GB at this time */
> +	if (e->efi_memmap_hi) {
> +		warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n");
> +		return false;
> +	}
> +	pmap =  e->efi_memmap;
> +#else
> +	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
> +#endif
> +
> +	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> +	for (i = 0; i < nr_desc; i++) {
> +		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
> +		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> +			region.start = md->phys_addr;
> +			region.size = md->num_pages << EFI_PAGE_SHIFT;
> +			process_mem_region(&region, minimum, image_size);
> +			efi_mirror_found = true;
> +
> +			if (slot_area_index == MAX_SLOT_AREA) {
> +				debug_putstr("Aborted EFI scan (slot_areas full)!\n");
> +				break;
> +			}
> +		}
> +	}
> +
> +	return efi_mirror_found;
> +}
> +#else
> +static inline bool
> +process_efi_entries(unsigned long minimum, unsigned long image_size)
> +{
> +	return false;
> +}
> +#endif
> +
>  static void process_e820_entries(unsigned long minimum,
>  				 unsigned long image_size)
>  {
> @@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
>  {
>  	/* Check if we had too many memmaps. */
>  	if (memmap_too_large) {
> -		debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
> +		debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
>  		return 0;
>  	}
>  
>  	/* Make sure minimum is aligned. */
>  	minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>  
> +	if (process_efi_entries(minimum, image_size))
> +		return slots_fetch_random();
> +
>  	process_e820_entries(minimum, image_size);
>  	return slots_fetch_random();
>  }
> @@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
>  	 */
>  	min_addr = min(*output, 512UL << 20);
>  
> -	/* Walk e820 and find a random address. */
> +	/* Walk available memory entries to find a random address. */
>  	random_addr = find_random_phys_addr(min_addr, output_size);
>  	if (!random_addr) {
>  		warn("Physical KASLR disabled: no suitable memory region!");
> -- 
> 2.5.5
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions
  2017-08-17 13:04   ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
@ 2017-08-18 15:10       ` Ard Biesheuvel
  0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-08-18 15:10 UTC (permalink / raw)
  To: Baoquan He
  Cc: Ingo Molnar, linux-kernel, Kees Cook, tglx, hpa, Taku Izumi,
	fanc.fnst, Thomas Garnier, n-horiguchi, linux-efi, x86,
	Matt Fleming

On 17 August 2017 at 14:04, Baoquan He <bhe@redhat.com> wrote:
> On 08/14/17 at 10:54pm, Baoquan He wrote:
>> Currently KASLR will parse all e820 entries of RAM type and add all
>> candidate position into slots array. Then we will choose one slot
>> randomly as the new position which kernel will be decompressed into
>> and run at.
>>
>> On system with EFI enabled, e820 memory regions are coming from EFI
>> memory regions by combining adjacent regions. While these EFI memory
>> regions have more attributes to mark their different use. Mirror
>> attribute is such kind. The physical memory region whose descriptors
>> in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are
>> mirrored. The address range mirroring feature of kernel arranges such
>> mirror region into normal zone and other region into movable zone. And
>> with mirroring feature enabled, the code and date of kernel can only be
>> located in more reliable mirror region. However, the current KASLR code
>> doesn't check EFI memory entries, and could choose new position in
>> non-mirrored region. This will break the functionality of the address
>> range mirroring feature.
>
> Thanks a lot for helping improving the patch log, Ingo! Will pay more
> attention to the description in words and paragraph partition of log.
>
>>
>> So if EFI is detected, iterate EFI memory map and pick the mirror region
>> to process for adding candidate of randomization slot. If EFI is disabled
>> or no mirror region found, still process e820 memory map.
>>
>> Signed-off-by: Baoquan He <bhe@redhat.com>


Could the x86 people on cc either take these directly, or indicate
whether they are ok with this going in via the EFI tree?

Thanks.

>> ---
>>  arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 99c7194f7ea6..7de23bb279ce 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -37,7 +37,9 @@
>>  #include <linux/uts.h>
>>  #include <linux/utsname.h>
>>  #include <linux/ctype.h>
>> +#include <linux/efi.h>
>>  #include <generated/utsrelease.h>
>> +#include <asm/efi.h>
>>
>>  /* Macros used by the included decompressor code below. */
>>  #define STATIC
>> @@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
>>       }
>>  }
>>
>> +#ifdef CONFIG_EFI
>> +/*
>> + * Returns true if mirror region found (and must have been processed
>> + * for slots adding)
>> + */
>> +static bool
>> +process_efi_entries(unsigned long minimum, unsigned long image_size)
>> +{
>> +     struct efi_info *e = &boot_params->efi_info;
>> +     bool efi_mirror_found = false;
>> +     struct mem_vector region;
>> +     efi_memory_desc_t *md;
>> +     unsigned long pmap;
>> +     char *signature;
>> +     u32 nr_desc;
>> +     int i;
>> +
>> +     signature = (char *)&e->efi_loader_signature;
>> +     if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
>> +         strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>> +             return false;
>> +
>> +#ifdef CONFIG_X86_32
>> +     /* Can't handle data above 4GB at this time */
>> +     if (e->efi_memmap_hi) {
>> +             warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n");
>> +             return false;
>> +     }
>> +     pmap =  e->efi_memmap;
>> +#else
>> +     pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
>> +#endif
>> +
>> +     nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
>> +     for (i = 0; i < nr_desc; i++) {
>> +             md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
>> +             if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>> +                     region.start = md->phys_addr;
>> +                     region.size = md->num_pages << EFI_PAGE_SHIFT;
>> +                     process_mem_region(&region, minimum, image_size);
>> +                     efi_mirror_found = true;
>> +
>> +                     if (slot_area_index == MAX_SLOT_AREA) {
>> +                             debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>> +                             break;
>> +                     }
>> +             }
>> +     }
>> +
>> +     return efi_mirror_found;
>> +}
>> +#else
>> +static inline bool
>> +process_efi_entries(unsigned long minimum, unsigned long image_size)
>> +{
>> +     return false;
>> +}
>> +#endif
>> +
>>  static void process_e820_entries(unsigned long minimum,
>>                                unsigned long image_size)
>>  {
>> @@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
>>  {
>>       /* Check if we had too many memmaps. */
>>       if (memmap_too_large) {
>> -             debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
>> +             debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
>>               return 0;
>>       }
>>
>>       /* Make sure minimum is aligned. */
>>       minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>>
>> +     if (process_efi_entries(minimum, image_size))
>> +             return slots_fetch_random();
>> +
>>       process_e820_entries(minimum, image_size);
>>       return slots_fetch_random();
>>  }
>> @@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
>>        */
>>       min_addr = min(*output, 512UL << 20);
>>
>> -     /* Walk e820 and find a random address. */
>> +     /* Walk available memory entries to find a random address. */
>>       random_addr = find_random_phys_addr(min_addr, output_size);
>>       if (!random_addr) {
>>               warn("Physical KASLR disabled: no suitable memory region!");
>> --
>> 2.5.5
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions
@ 2017-08-18 15:10       ` Ard Biesheuvel
  0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-08-18 15:10 UTC (permalink / raw)
  To: Baoquan He
  Cc: Ingo Molnar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Kees Cook,
	tglx-hfZtesqFncYOwBW4kG4KsQ, hpa-YMNOUZJC4hwAvxtiuMwx3w,
	Taku Izumi, fanc.fnst-BthXqXjhjHXQFUHtdCDX3A, Thomas Garnier,
	n-horiguchi-PaJj6Psr51x8UrSeD/g0lQ,
	linux-efi-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
	Matt Fleming

On 17 August 2017 at 14:04, Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 08/14/17 at 10:54pm, Baoquan He wrote:
>> Currently KASLR will parse all e820 entries of RAM type and add all
>> candidate position into slots array. Then we will choose one slot
>> randomly as the new position which kernel will be decompressed into
>> and run at.
>>
>> On system with EFI enabled, e820 memory regions are coming from EFI
>> memory regions by combining adjacent regions. While these EFI memory
>> regions have more attributes to mark their different use. Mirror
>> attribute is such kind. The physical memory region whose descriptors
>> in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are
>> mirrored. The address range mirroring feature of kernel arranges such
>> mirror region into normal zone and other region into movable zone. And
>> with mirroring feature enabled, the code and date of kernel can only be
>> located in more reliable mirror region. However, the current KASLR code
>> doesn't check EFI memory entries, and could choose new position in
>> non-mirrored region. This will break the functionality of the address
>> range mirroring feature.
>
> Thanks a lot for helping improving the patch log, Ingo! Will pay more
> attention to the description in words and paragraph partition of log.
>
>>
>> So if EFI is detected, iterate EFI memory map and pick the mirror region
>> to process for adding candidate of randomization slot. If EFI is disabled
>> or no mirror region found, still process e820 memory map.
>>
>> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


Could the x86 people on cc either take these directly, or indicate
whether they are ok with this going in via the EFI tree?

Thanks.

>> ---
>>  arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 99c7194f7ea6..7de23bb279ce 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -37,7 +37,9 @@
>>  #include <linux/uts.h>
>>  #include <linux/utsname.h>
>>  #include <linux/ctype.h>
>> +#include <linux/efi.h>
>>  #include <generated/utsrelease.h>
>> +#include <asm/efi.h>
>>
>>  /* Macros used by the included decompressor code below. */
>>  #define STATIC
>> @@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
>>       }
>>  }
>>
>> +#ifdef CONFIG_EFI
>> +/*
>> + * Returns true if mirror region found (and must have been processed
>> + * for slots adding)
>> + */
>> +static bool
>> +process_efi_entries(unsigned long minimum, unsigned long image_size)
>> +{
>> +     struct efi_info *e = &boot_params->efi_info;
>> +     bool efi_mirror_found = false;
>> +     struct mem_vector region;
>> +     efi_memory_desc_t *md;
>> +     unsigned long pmap;
>> +     char *signature;
>> +     u32 nr_desc;
>> +     int i;
>> +
>> +     signature = (char *)&e->efi_loader_signature;
>> +     if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
>> +         strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>> +             return false;
>> +
>> +#ifdef CONFIG_X86_32
>> +     /* Can't handle data above 4GB at this time */
>> +     if (e->efi_memmap_hi) {
>> +             warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n");
>> +             return false;
>> +     }
>> +     pmap =  e->efi_memmap;
>> +#else
>> +     pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
>> +#endif
>> +
>> +     nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
>> +     for (i = 0; i < nr_desc; i++) {
>> +             md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
>> +             if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>> +                     region.start = md->phys_addr;
>> +                     region.size = md->num_pages << EFI_PAGE_SHIFT;
>> +                     process_mem_region(&region, minimum, image_size);
>> +                     efi_mirror_found = true;
>> +
>> +                     if (slot_area_index == MAX_SLOT_AREA) {
>> +                             debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>> +                             break;
>> +                     }
>> +             }
>> +     }
>> +
>> +     return efi_mirror_found;
>> +}
>> +#else
>> +static inline bool
>> +process_efi_entries(unsigned long minimum, unsigned long image_size)
>> +{
>> +     return false;
>> +}
>> +#endif
>> +
>>  static void process_e820_entries(unsigned long minimum,
>>                                unsigned long image_size)
>>  {
>> @@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
>>  {
>>       /* Check if we had too many memmaps. */
>>       if (memmap_too_large) {
>> -             debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
>> +             debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
>>               return 0;
>>       }
>>
>>       /* Make sure minimum is aligned. */
>>       minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>>
>> +     if (process_efi_entries(minimum, image_size))
>> +             return slots_fetch_random();
>> +
>>       process_e820_entries(minimum, image_size);
>>       return slots_fetch_random();
>>  }
>> @@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
>>        */
>>       min_addr = min(*output, 512UL << 20);
>>
>> -     /* Walk e820 and find a random address. */
>> +     /* Walk available memory entries to find a random address. */
>>       random_addr = find_random_phys_addr(min_addr, output_size);
>>       if (!random_addr) {
>>               warn("Physical KASLR disabled: no suitable memory region!");
>> --
>> 2.5.5
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions
  2017-08-18 15:10       ` Ard Biesheuvel
  (?)
@ 2017-08-19  1:22       ` Baoquan He
  -1 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2017-08-19  1:22 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Ingo Molnar, linux-kernel, Kees Cook, tglx, hpa, Taku Izumi,
	fanc.fnst, Thomas Garnier, n-horiguchi, linux-efi, x86,
	Matt Fleming

On 08/18/17 at 04:10pm, Ard Biesheuvel wrote:
> On 17 August 2017 at 14:04, Baoquan He <bhe@redhat.com> wrote:

> > Thanks a lot for helping improving the patch log, Ingo! Will pay more
> > attention to the description in words and paragraph partition of log.
> >
> >>
> >> So if EFI is detected, iterate EFI memory map and pick the mirror region
> >> to process for adding candidate of randomization slot. If EFI is disabled
> >> or no mirror region found, still process e820 memory map.
> >>
> >> Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> 
> Could the x86 people on cc either take these directly, or indicate
> whether they are ok with this going in via the EFI tree?

Thanks for looking into this, Ard. I saw Ingo has put these into
tip/x86/boot.

> 
> >> ---
> >>  arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 66 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> >> index 99c7194f7ea6..7de23bb279ce 100644
> >> --- a/arch/x86/boot/compressed/kaslr.c
> >> +++ b/arch/x86/boot/compressed/kaslr.c
> >> @@ -37,7 +37,9 @@
> >>  #include <linux/uts.h>
> >>  #include <linux/utsname.h>
> >>  #include <linux/ctype.h>
> >> +#include <linux/efi.h>
> >>  #include <generated/utsrelease.h>
> >> +#include <asm/efi.h>
> >>
> >>  /* Macros used by the included decompressor code below. */
> >>  #define STATIC
> >> @@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
> >>       }
> >>  }
> >>
> >> +#ifdef CONFIG_EFI
> >> +/*
> >> + * Returns true if mirror region found (and must have been processed
> >> + * for slots adding)
> >> + */
> >> +static bool
> >> +process_efi_entries(unsigned long minimum, unsigned long image_size)
> >> +{
> >> +     struct efi_info *e = &boot_params->efi_info;
> >> +     bool efi_mirror_found = false;
> >> +     struct mem_vector region;
> >> +     efi_memory_desc_t *md;
> >> +     unsigned long pmap;
> >> +     char *signature;
> >> +     u32 nr_desc;
> >> +     int i;
> >> +
> >> +     signature = (char *)&e->efi_loader_signature;
> >> +     if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> >> +         strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> >> +             return false;
> >> +
> >> +#ifdef CONFIG_X86_32
> >> +     /* Can't handle data above 4GB at this time */
> >> +     if (e->efi_memmap_hi) {
> >> +             warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n");
> >> +             return false;
> >> +     }
> >> +     pmap =  e->efi_memmap;
> >> +#else
> >> +     pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
> >> +#endif
> >> +
> >> +     nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> >> +     for (i = 0; i < nr_desc; i++) {
> >> +             md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
> >> +             if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> >> +                     region.start = md->phys_addr;
> >> +                     region.size = md->num_pages << EFI_PAGE_SHIFT;
> >> +                     process_mem_region(&region, minimum, image_size);
> >> +                     efi_mirror_found = true;
> >> +
> >> +                     if (slot_area_index == MAX_SLOT_AREA) {
> >> +                             debug_putstr("Aborted EFI scan (slot_areas full)!\n");
> >> +                             break;
> >> +                     }
> >> +             }
> >> +     }
> >> +
> >> +     return efi_mirror_found;
> >> +}
> >> +#else
> >> +static inline bool
> >> +process_efi_entries(unsigned long minimum, unsigned long image_size)
> >> +{
> >> +     return false;
> >> +}
> >> +#endif
> >> +
> >>  static void process_e820_entries(unsigned long minimum,
> >>                                unsigned long image_size)
> >>  {
> >> @@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
> >>  {
> >>       /* Check if we had too many memmaps. */
> >>       if (memmap_too_large) {
> >> -             debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
> >> +             debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
> >>               return 0;
> >>       }
> >>
> >>       /* Make sure minimum is aligned. */
> >>       minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
> >>
> >> +     if (process_efi_entries(minimum, image_size))
> >> +             return slots_fetch_random();
> >> +
> >>       process_e820_entries(minimum, image_size);
> >>       return slots_fetch_random();
> >>  }
> >> @@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
> >>        */
> >>       min_addr = min(*output, 512UL << 20);
> >>
> >> -     /* Walk e820 and find a random address. */
> >> +     /* Walk available memory entries to find a random address. */
> >>       random_addr = find_random_phys_addr(min_addr, output_size);
> >>       if (!random_addr) {
> >>               warn("Physical KASLR disabled: no suitable memory region!");
> >> --
> >> 2.5.5
> >>

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-08-19  1:22 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-14 14:54 [PATCH v9 0/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
2017-08-14 14:54 ` [PATCH v9 1/2] efi: Introduce efi_early_memdesc_ptr to get pointer to memmap descriptor Baoquan He
2017-08-16 11:37   ` Matt Fleming
2017-08-16 11:37     ` Matt Fleming
2017-08-16 13:18     ` Baoquan He
2017-08-16 13:18       ` Baoquan He
2017-08-16 13:46   ` [PATCH v10 " Baoquan He
2017-08-17 10:20     ` [tip:x86/boot] " tip-bot for Baoquan He
2017-08-14 14:54 ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
2017-08-17 10:21   ` [tip:x86/boot] x86/boot/KASLR: Prefer mirrored memory regions for the kernel physical address tip-bot for Baoquan He
2017-08-17 13:04   ` [PATCH v9 2/2] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Baoquan He
2017-08-18 15:10     ` Ard Biesheuvel
2017-08-18 15:10       ` Ard Biesheuvel
2017-08-19  1:22       ` Baoquan He

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.