Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752320AbdLGBKF (ORCPT ); Wed, 6 Dec 2017 20:10:05 -0500 Received: from mail.cn.fujitsu.com ([183.91.158.132]:58264 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752047AbdLGBKE (ORCPT ); Wed, 6 Dec 2017 20:10:04 -0500 X-IronPort-AV: E=Sophos;i="5.43,368,1503331200"; d="scan'208";a="30784259" Date: Thu, 7 Dec 2017 09:09:36 +0800 From: Chao Fan To: Kees Cook CC: Baoquan He , LKML , X86 ML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , , , , Dou Liyang Subject: Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node Message-ID: <20171207010935.GC28884@localhost.localdomain> References: <20171205085200.9528-1-fanc.fnst@cn.fujitsu.com> <20171205085200.9528-3-fanc.fnst@cn.fujitsu.com> <20171206092800.GK15074@x1> <20171206100238.GB28884@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) X-Originating-IP: [10.167.225.56] X-yoursite-MailScanner-ID: BFABD486A776.AF56F X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: fanc.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4421 Lines: 118 On Wed, Dec 06, 2017 at 04:11:04PM -0800, Kees Cook wrote: >On Wed, Dec 6, 2017 at 2:02 AM, Chao Fan 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 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(). Thanks for your review, I will change and send the new version. Thanks, Chao Fan > >-Kees > >-- >Kees Cook >Pixel Security > >