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

[Kees Cook]

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