lkml.org 
[lkml]   [2017]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node
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@cn.fujitsu.com> 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@cn.fujitsu.com> 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
>
>


\
 
 \ /
  Last update: 2017-12-07 02:10    [W:0.132 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site