Messages in this thread | | | From | Kees Cook <> | Date | Wed, 6 Dec 2017 16:11:04 -0800 | Subject | Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node |
| |
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(®ion, &overlap)) { > store_slot_info(®ion, 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
| |