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

[Chao Fan]

On Wed, Dec 06, 2017 at 04:11:04PM -0800, Kees Cook wrote:
>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().

Thanks for your review, I will change and send the new version.

Thanks,
Chao Fan

>
>-Kees
>
>-- 
>Kees Cook
>Pixel Security
>
>