From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753228AbdGJCs7 (ORCPT ); Sun, 9 Jul 2017 22:48:59 -0400 Received: from cn.fujitsu.com ([59.151.112.132]:26797 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753128AbdGJCs5 (ORCPT ); Sun, 9 Jul 2017 22:48:57 -0400 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="21068224" Date: Mon, 10 Jul 2017 10:48:49 +0800 From: Chao Fan To: Baoquan He CC: Kees Cook , LKML , "x86@kernel.org" , Matt Fleming , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , , Thomas Garnier Subject: Re: [PATCH v4 4/4] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Message-ID: <20170710024848.GA27579@localhost.localdomain> References: <1499603862-11516-1-git-send-email-bhe@redhat.com> <1499603862-11516-5-git-send-email-bhe@redhat.com> <20170710014729.GC13560@x1> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20170710014729.GC13560@x1> User-Agent: Mutt/1.8.3 (2017-05-23) X-Originating-IP: [10.167.226.75] X-yoursite-MailScanner-ID: E61D44D14A75.ACB66 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 On Mon, Jul 10, 2017 at 09:47:29AM +0800, Baoquan He wrote: >On 07/09/17 at 07:11am, Kees Cook wrote: >> On Sun, Jul 9, 2017 at 5:37 AM, Baoquan He wrote: >> > Signed-off-by: Baoquan He >> > +/* Marks if efi mirror regions have been found and handled. */ >> > +static bool efi_mirror_found; >> >> I think this is only ever checked once? How about having >> process_efi_entries return bool to indicate if mirror was found? Also, >> that function should be behind #ifdef. Let's do something like this: >> >> >> > + >> > +static void process_efi_entries(unsigned long minimum, >> > + unsigned long image_size) >> >> #ifdef CONFIG_EFI >> /* Returns true if mirror region found (and further scanning should stop) */ > >Well, here it might be not like that. The mirror regions could be multiple, >we need find and process each of them to add slots. Yes, in my test, I found the memory situation is like this: 0-1325M: mirror (this scope contains tens of entries) 1532M-2303M: non-mirror 4G-128G: non-mirror 129G-154G: mirror 154G-231G: non-mirror Thanks, Chao Fan > >> static bool process_efi_entries(...) >> { >> ... >> } >> #else >> static inline bool process_efi_entries(...) >> { >> return false; >> } >> #endif >> >> Then: >> >> > +#ifdef CONFIG_EFI >> > + process_efi_entries(minimum, image_size); >> > + if (efi_mirror_found) >> > + return slots_fetch_random(); >> > +#endif >> >> Can become: >> >> if (process_efi_entries(minimum, image_size)) >> return slots_fetch_random() >> >> and no #ifndef needed here. >> >> > + >> > process_e820_entries(minimum, image_size); >> > return slots_fetch_random(); >> > } >> > @@ -652,7 +708,7 @@ void choose_random_location(unsigned long input, >> > */ >> > min_addr = min(*output, 512UL << 20); >> > >> > - /* Walk e820 and find a random address. */ >> > + /* Walk available memory entries to find a random address. */ >> > random_addr = find_random_phys_addr(min_addr, output_size); >> > if (!random_addr) { >> > warn("Physical KASLR disabled: no suitable memory region!"); >> > -- >> > 2.5.5 >> > >> >> Otherwise, if the EFI logic is good, this looks sensible. >> >> Thanks for splitting up the patches! >> >> -Kees >> >> -- >> Kees Cook >> Pixel Security > >