x86/boot/KASLR: Restrict kernel to be randomized in mirror regions

The original function process_e820_entry() only takes care of each e820 entry passed.

And move the E820_TYPE_RAM checking logic into process_e820_entries().

And remove the redundent local variable 'addr' definition in find_random_phys_addr().

Signed-off-by Baoquan He <[email protected]>

---
 arch/x86/boot/compressed/kaslr.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

arch/x86/boot/compressed/kaslr.c +21 -18

index 91f27ab970ef..1485f48aeda1 100644
@@ -488,10 +488,6 @@ static void process_e820_entry(struct boot_e820_entry *entry,
        unsigned long start_orig, end;
        struct boot_e820_entry cur_entry;
 
-       /* Skip non-RAM entries. */
-       if (entry->type != E820_TYPE_RAM)
-               return;
-
        /* On 32-bit, ignore entries entirely above our maximum. */
        if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
                return;

@@ -562,12 +558,29 @@ static void process_e820_entry(struct boot_e820_entry *entry,
        }
 }
 
-static unsigned long find_random_phys_addr(unsigned long minimum,
-                                          unsigned long image_size)
+static void process_e820_entries(unsigned long minimum,
+                                unsigned long image_size)
 {
        int i;
-       unsigned long addr;
+       struct boot_e820_entry *entry;
+
+       /* Verify potential e820 positions, appending to slots list. */
+       for (i = 0; i < boot_params->e820_entries; i++) {
+               entry = &boot_params->e820_table[i];
+               /* Skip non-RAM entries. */
+               if (entry->type != E820_TYPE_RAM)
+                       continue;
+               process_e820_entry(entry, minimum, image_size);
+               if (slot_area_index == MAX_SLOT_AREA) {
+                       debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+                       break;
+               }
+       }
+}
 
+static unsigned long find_random_phys_addr(unsigned long minimum,
+                                          unsigned long image_size)
+{
        /* Check if we had too many memmaps. */
        if (memmap_too_large) {
                debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");

@@ -577,16 +590,7 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
        /* Make sure minimum is aligned. */
        minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
 
-       /* Verify potential e820 positions, appending to slots list. */
-       for (i = 0; i < boot_params->e820_entries; i++) {
-               process_e820_entry(&boot_params->e820_table[i], minimum,
-                                  image_size);
-               if (slot_area_index == MAX_SLOT_AREA) {
-                       debug_putstr("Aborted e820 scan (slot_areas full)!\n");
-                       break;
-               }
-       }
-
+       process_e820_entries(minimum, image_size);
        return slots_fetch_random();
 }
 
--

2.5.5

This makes process_e820_entry() be able to process any kind of memory region.

Signed-off-by Baoquan He <[email protected]>

---
 arch/x86/boot/compressed/kaslr.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

arch/x86/boot/compressed/kaslr.c +14 -12

index 1485f48aeda1..36ff9f729c43 100644
@@ -479,31 +479,31 @@ static unsigned long slots_fetch_random(void)
        return 0;
 }
 
-static void process_e820_entry(struct boot_e820_entry *entry,
+static void process_e820_entry(struct mem_vector *entry,
                               unsigned long minimum,
                               unsigned long image_size)
 {
        struct mem_vector region, overlap;
        struct slot_area slot_area;
        unsigned long start_orig, end;
-       struct boot_e820_entry cur_entry;
+       struct mem_vector cur_entry;
 
        /* On 32-bit, ignore entries entirely above our maximum. */
-       if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
+       if (IS_ENABLED(CONFIG_X86_32) && entry->start >= KERNEL_IMAGE_SIZE)
                return;
 
        /* Ignore entries entirely below our minimum. */
-       if (entry->addr + entry->size < minimum)
+       if (entry->start + entry->size < minimum)
                return;
 
        /* Ignore entries above memory limit */
-       end = min(entry->size + entry->addr, mem_limit);
-       if (entry->addr >= end)
+       end = min(entry->size + entry->start, mem_limit);
+       if (entry->start >= end)
                return;
-       cur_entry.addr = entry->addr;
-       cur_entry.size = end - entry->addr;
+       cur_entry.start = entry->start;
+       cur_entry.size = end - entry->start;
 
-       region.start = cur_entry.addr;
+       region.start = cur_entry.start;
        region.size = cur_entry.size;
 
        /* Give up if slot area array is full. */

@@ -518,7 +518,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
                region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
 
                /* Did we raise the address above this e820 region? */
-               if (region.start > cur_entry.addr + cur_entry.size)
+               if (region.start > cur_entry.start + cur_entry.size)
                        return;
 
                /* Reduce size by any delta from the original address. */

@@ -562,6 +562,7 @@ static void process_e820_entries(unsigned long minimum,
                                 unsigned long image_size)
 {
        int i;
+       struct mem_vector region;
        struct boot_e820_entry *entry;
 
        /* Verify potential e820 positions, appending to slots list. */

@@ -570,7 +571,9 @@ static void process_e820_entries(unsigned long minimum,
                /* Skip non-RAM entries. */
                if (entry->type != E820_TYPE_RAM)
                        continue;
-               process_e820_entry(entry, minimum, image_size);
+               region.start = entry->addr;
+               region.size = entry->size;
+               process_e820_entry(&region, minimum, image_size);
                if (slot_area_index == MAX_SLOT_AREA) {
                        debug_putstr("Aborted e820 scan (slot_areas full)!\n");
                        break;
--

2.5.5

Now process_e820_entry() is not limited to e820 entry processing, rename it to process_mem_region(). And adjust the code comment accordingly.

Signed-off-by Baoquan He <[email protected]>

---
 arch/x86/boot/compressed/kaslr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/x86/boot/compressed/kaslr.c +3 -4

index 36ff9f729c43..99c7194f7ea6 100644
@@ -479,7 +479,7 @@ static unsigned long slots_fetch_random(void)
        return 0;
 }
 
-static void process_e820_entry(struct mem_vector *entry,
+static void process_mem_region(struct mem_vector *entry,
                               unsigned long minimum,
                               unsigned long image_size)
 {

@@ -517,7 +517,7 @@ static void process_e820_entry(struct mem_vector *entry,
                /* Potentially raise address to meet alignment needs. */
                region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
 
-               /* Did we raise the address above this e820 region? */
+               /* Did we raise the address above the passed in memory entry? */
                if (region.start > cur_entry.start + cur_entry.size)
                        return;
 

@@ -573,7 +573,7 @@ static void process_e820_entries(unsigned long minimum,
                        continue;
                region.start = entry->addr;
                region.size = entry->size;
-               process_e820_entry(&region, minimum, image_size);
+               process_mem_region(&region, minimum, image_size);
                if (slot_area_index == MAX_SLOT_AREA) {
                        debug_putstr("Aborted e820 scan (slot_areas full)!\n");
                        break;
--

2.5.5

Kernel text may be located in non-mirror regions (movable zone) when both address range mirroring feature and KASLR are enabled.

The address range mirroring feature arranges such mirror region into normal zone and other region into movable zone in order to locate kernel code and data in mirror region. The physical memory region whose descriptors in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are mirrored.

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 <[email protected]>

---
 arch/x86/boot/compressed/kaslr.c | 60 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)

arch/x86/boot/compressed/kaslr.c +2 -0

index 99c7194f7ea6..7376b3473758 100644
@@ -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

arch/x86/boot/compressed/kaslr.c +2 -2

+
        process_e820_entries(minimum, image_size);
        return slots_fetch_random();
 }

@@ -652,7 +708,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

Quoted 36 lines · Baoquan He

Kernel text may be located in non-mirror regions (movable zone) when both address range mirroring feature and KASLR are enabled.

The address range mirroring feature arranges such mirror region into normal zone and other region into movable zone in order to locate kernel code and data in mirror region. The physical memory region whose descriptors in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are mirrored.

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 <[email protected]>

---
 arch/x86/boot/compressed/kaslr.c | 60 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)

arch/x86/boot/compressed/kaslr.c +2 -0

index 99c7194f7ea6..7376b3473758 100644
@@ -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,54 @@ static void process_mem_region(struct mem_vector *entry,
        }
 }
 

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 564

Quoted 2 lines

arch/x86/boot/compressed/kaslr.c +2 -0

+/* Marks if efi mirror regions have been found and handled. */
+static bool efi_mirror_found;

I think this is only ever checked once? How about having process_efi_entries return bool to indicate if mirror was found? Also, that function should be behind #ifdef. Let's do something like this:

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 567

Quoted 5 lines

arch/x86/boot/compressed/kaslr.c +3 -0

+
+static void process_efi_entries(unsigned long minimum,
+                               unsigned long image_size)

#ifdef CONFIG_EFI
/* Returns true if mirror region found (and further scanning should stop) */ static bool process_efi_entries(...)
{
...
}
#else
static inline bool process_efi_entries(...)

{
    return false;
}

#endif

Then:

arch/x86/boot/compressed/kaslr.c static unsigned long find_random_phys_addr(unsigned long minimum, · line 650

Quoted 5 lines

arch/x86/boot/compressed/kaslr.c +5 -0

+#ifdef CONFIG_EFI
+       process_efi_entries(minimum, image_size);
+       if (efi_mirror_found)
+               return slots_fetch_random();
+#endif

Can become:

if (process_efi_entries(minimum, image_size))
return slots_fetch_random()

and no #ifndef needed here.

Quoted 14 lines

arch/x86/boot/compressed/kaslr.c +2 -2

+
        process_e820_entries(minimum, image_size);
        return slots_fetch_random();
 }

@@ -652,7 +708,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!");
--

Quoted 2 lines

2.5.5

Otherwise, if the EFI logic is good, this looks sensible.

Thanks for splitting up the patches!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 564

Quoted 3 lines · Baoquan He

Signed-off-by Baoquan He <[email protected]>

arch/x86/boot/compressed/kaslr.c +2 -0

+/* Marks if efi mirror regions have been found and handled. */
+static bool efi_mirror_found;

I think this is only ever checked once? How about having process_efi_entries return bool to indicate if mirror was found? Also, that function should be behind #ifdef. Let's do something like this:

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 567

Quoted 3 lines

arch/x86/boot/compressed/kaslr.c +3 -0

+
+static void process_efi_entries(unsigned long minimum,
+                               unsigned long image_size)

#ifdef CONFIG_EFI
/* Returns true if mirror region found (and further scanning should stop) */

static bool process_efi_entries(...)
{
...
}
#else
static inline bool process_efi_entries(...)

{
    return false;
}

#endif

Then:

arch/x86/boot/compressed/kaslr.c static unsigned long find_random_phys_addr(unsigned long minimum, · line 650

Quoted 5 lines

arch/x86/boot/compressed/kaslr.c +5 -0

+#ifdef CONFIG_EFI
+       process_efi_entries(minimum, image_size);
+       if (efi_mirror_found)
+               return slots_fetch_random();
+#endif

Can become:

if (process_efi_entries(minimum, image_size))
return slots_fetch_random()

and no #ifndef needed here.

Quoted 14 lines

arch/x86/boot/compressed/kaslr.c +2 -2

+
        process_e820_entries(minimum, image_size);
        return slots_fetch_random();
 }

@@ -652,7 +708,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!");
--

Quoted 2 lines

2.5.5

Otherwise, if the EFI logic is good, this looks sensible.

Thanks for splitting up the patches!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Quoted 10 lines · Kees Cook

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 564

Quoted 3 lines · Baoquan He

Signed-off-by Baoquan He <[email protected]>

arch/x86/boot/compressed/kaslr.c +2 -0

+/* Marks if efi mirror regions have been found and handled. */
+static bool efi_mirror_found;

I think this is only ever checked once? How about having process_efi_entries return bool to indicate if mirror was found? Also, that function should be behind #ifdef. Let's do something like this:

Quoted 6 lines

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 567

Quoted 3 lines

arch/x86/boot/compressed/kaslr.c +3 -0

+
+static void process_efi_entries(unsigned long minimum,
+                               unsigned long image_size)

#ifdef CONFIG_EFI
/* Returns true if mirror region found (and further scanning should stop) */

Well, here it might be not like that. The mirror regions could be multiple, we need find and process each of them to add slots.

Quoted 53 lines

static bool process_efi_entries(...)
{
...
}
#else
static inline bool process_efi_entries(...)

{
    return false;
}

#endif

Then:

arch/x86/boot/compressed/kaslr.c static unsigned long find_random_phys_addr(unsigned long minimum, · line 650

Quoted 5 lines

arch/x86/boot/compressed/kaslr.c +5 -0

+#ifdef CONFIG_EFI
+       process_efi_entries(minimum, image_size);
+       if (efi_mirror_found)
+               return slots_fetch_random();
+#endif

Can become:

if (process_efi_entries(minimum, image_size))
return slots_fetch_random()

and no #ifndef needed here.

Quoted 14 lines

arch/x86/boot/compressed/kaslr.c +2 -2

+
        process_e820_entries(minimum, image_size);
        return slots_fetch_random();
 }

@@ -652,7 +708,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!");
--

Quoted 2 lines

2.5.5

Otherwise, if the EFI logic is good, this looks sensible.

Thanks for splitting up the patches!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

From 1cec45dc65090f6d17bf3499b8904efc1822082e Mon Sep 17 00:00:00 2001