Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

From: Chao Fan
Date: Wed Dec 06 2017 - 05:03:14 EST

On Wed, Dec 06, 2017 at 05:28:00PM +0800, Baoquan He wrote:
>On 12/05/17 at 11:40am, Kees Cook wrote:
>> On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@xxxxxxxxxxxxxx> wrote:
>> > If there is no immovable memory region specified, go on the old code.
>> > There are several conditons:
>> > 1. CONFIG_MEMORY_HOTPLUG is not specified to y.
>> > 2. immovable_mem= is not specified.
>> >
>> > Otherwise, calculate the intersecting between memmap entry and
>> > immovable memory.
>>
>> Instead of copy/pasting code between process_efi_entries() and
>> process_e820_entries(), I'd rather that process_mem_region() is
>> modified to deal with immovable regions.
>
>If put it into process_mem_region(), one level of loop is added. How

Yes, one new loop will add ahead of the while() in process_mem_region
then the code may look like:

@@ -509,6 +555,24 @@ static void process_mem_region(struct mem_vector *entry,
region.start = cur_entry.start;
region.size = cur_entry.size;

+#ifdef CONFIG_MEMORY_HOTPLUG
+next:
+ if (num_immovable_mem > 0) {
+ unsigned long long start, reg_end;
+
+ if (!mem_overlaps(&entry, &immovable_mem[i]))
+ goto out;
+
+ start = immovable_mem[i].start;
+ end = start + immovable_mem[i].size;
+
+ region.start = clamp(cur_entry.start, start, end);
+ reg_end = clamp(cur_entry.start + cur_entry.size, start, end);
+
+ region.size = region_end - region.start;
+ }
+#endif
+
/* Give up if slot area array is full. */
while (slot_area_index < MAX_SLOT_AREA) {
start_orig = region.start;
@@ -522,7 +586,7 @@ static void process_mem_region(struct mem_vector *entry,

/* Did we raise the address above the passed in memory entry? */
if (region.start > cur_entry.start + cur_entry.size)
- return;
+ goto out;

/* Reduce size by any delta from the original address. */
region.size -= region.start - start_orig;
@@ -534,12 +598,12 @@ static void process_mem_region(struct mem_vector *entry,

/* Return if region can't contain decompressed kernel */
if (region.size < image_size)
- return;
+ goto out;

/* If nothing overlaps, store the region and return. */
if (!mem_avoid_overlap(&region, &overlap)) {
store_slot_info(&region, image_size);
- return;
+ goto out;
}

/* Store beginning of region if holds at least image_size. */
@@ -553,12 +617,20 @@ static void process_mem_region(struct mem_vector *entry,

/* Return if overlap extends to or past end of region. */
if (overlap.start + overlap.size >= region.start + region.size)
- return;
+ goto out;

/* Clip off the overlapping region and start over. */
region.size -= overlap.start - region.start + overlap.size;
region.start = overlap.start + overlap.size;
}
+
+out:
+#ifdef CONFIG_MEMORY_HOTPLUG
+ i++;
+ if (i < num_immovable_mem)
+ goto next;
+#endif
+ return;
}

>about changing it like below. If no immovable_mem, just process the
>region in process_immovable_mem(). This we don't need to touch
>process_mem_region().

Yes, Baoquan's method will make all change be in one function.
Kees, how do you think, which is better?

Thanks,
Chao Fan

>
>
>From 9ae3f5ab0e2f129757495af2412bd52dcf86aa46 Mon Sep 17 00:00:00 2001
>From: Baoquan He <bhe@xxxxxxxxxx>
>Date: Wed, 6 Dec 2017 17:24:55 +0800
>Subject: [PATCH] KASLR: change code
>
>Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
>---
> arch/x86/boot/compressed/kaslr.c | 32 ++++++++++++++------------------
> 1 file changed, 14 insertions(+), 18 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 13d26b859c69..73b1562a7439 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -638,13 +638,23 @@ static bool process_immovable_mem(struct mem_vector region,
> unsigned long long minimum,
> unsigned long long image_size)
> {
>- int i;
>+ /* If no immovable_mem stored, use region directly */
>+ if (num_immovable_region == 0) {
>+ process_mem_region(&entry, minimum, image_size);
>+
>+ if (slot_area_index == MAX_SLOT_AREA) {
>+ debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>+ return 1;
>+ }
>+
>+ return 0;
>+ }
>
> /*
> * Walk all immovable regions, and filter the intersection
> * to process_mem_region.
> */
>- for (i = 0; i < num_immovable_region; i++) {
>+ for (int i = 0; i < num_immovable_region; i++) {
> struct mem_vector entry;
> unsigned long long start, end, entry_end;
>
>@@ -738,14 +748,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
> region.start = md->phys_addr;
> region.size = md->num_pages << EFI_PAGE_SHIFT;
>
>- /* If no immovable_mem stored, use region directly */
>- if (num_immovable_region == 0) {
>- process_mem_region(&region, minimum, image_size);
>- if (slot_area_index == MAX_SLOT_AREA) {
>- debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>- break;
>- }
>- } else if (process_immovable_mem(region, minimum, image_size))
>+ if (process_immovable_mem(region, minimum, image_size))
> break;
> }
> return true;
>@@ -774,14 +777,7 @@ static void process_e820_entries(unsigned long minimum,
> region.start = entry->addr;
> region.size = entry->size;
>
>- /* If no immovable_mem stored, use region directly */
>- if (num_immovable_region == 0) {
>- process_mem_region(&region, minimum, image_size);
>- if (slot_area_index == MAX_SLOT_AREA) {
>- debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>- break;
>- }
>- } else if (process_immovable_mem(region, minimum, image_size))
>+ if (process_immovable_mem(region, minimum, image_size))
> break;
> }
> }
>--
>2.5.5
>
>
>