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

From: Kees Cook
Date: Wed Dec 06 2017 - 19:11:11 EST


On Wed, Dec 6, 2017 at 2:02 AM, Chao Fan <fanc.fnst@xxxxxxxxxxxxxx> wrote:
> 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?

I prefer Baoquan's approach, though I don't like the function names.
:) Perhaps rename process_mem_region() to slots_count() (to match
slots_fetch_random()) and rename process_immovable_mem() to
process_mem_region().

-Kees

--
Kees Cook
Pixel Security