Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751578AbdGUKiD (ORCPT ); Fri, 21 Jul 2017 06:38:03 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:38667 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750737AbdGUKiB (ORCPT ); Fri, 21 Jul 2017 06:38:01 -0400 Date: Fri, 21 Jul 2017 12:37:57 +0200 From: Ingo Molnar To: Baoquan He Cc: linux-kernel@vger.kernel.org, x86@kernel.org, keescook@chromium.org, matt@codeblueprint.co.uk, tglx@linutronix.de, hpa@zytor.com, izumi.taku@jp.fujitsu.com, fanc.fnst@cn.fujitsu.com, thgarnie@google.com, n-horiguchi@ah.jp.nec.com Subject: Re: [PATCH v6 RESEND] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Message-ID: <20170721103757.hc74czr3mfunrv6c@gmail.com> References: <1500542189-15779-1-git-send-email-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1500542189-15779-1-git-send-email-bhe@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1724 Lines: 61 * Baoquan He wrote: > +/* > + * Returns true if mirror region found (and must have been processed > + * for slots adding) > + */ > +static bool process_efi_entries(unsigned long minimum, > + unsigned long image_size) Also, please don't break the line in the middle of the prototype. > +{ > + struct efi_info *e = &boot_params->efi_info; > + bool efi_mirror_found = false; > + struct mem_vector region; > + efi_memory_desc_t *md; > + unsigned long pmap; > + char *signature; > + u32 nr_desc; > + int i; > + > + signature = (char *)&boot_params->efi_info.efi_loader_signature; This is sloppy too: we already have '&boot_params->efi_info' in 'e', why do you duplicate it again, why not write 'e->efi_loader_signature'?? > + if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) && > + strncmp(signature, EFI64_LOADER_SIGNATURE, 4)) > + return false; > + > +#ifdef CONFIG_X86_32 > + /* Can't handle data above 4GB at this time */ > + if (e->efi_memmap_hi) { > + warn("Memory map is above 4GB, EFI should be disabled.\n"); > + return false; This kernel warning is pretty passive-aggressive: please explain what the problem is and how it can be resolved. > + } > + pmap = e->efi_memmap; > +#else > + pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > +#endif > + > + nr_desc = e->efi_memmap_size / e->efi_memdesc_size; > + for (i = 0; i < nr_desc; i++) { > + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size)); This looks unnecessarily obfuscated: why not initialize 'md' to 'pmap' when pmap is calculated and just use md[i]? > +static inline bool process_efi_entries(unsigned long minimum, > + unsigned long image_size) ugly linebreak again ... Thanks, Ingo