From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755315AbcFQJCX (ORCPT ); Fri, 17 Jun 2016 05:02:23 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33115 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752847AbcFQJCT (ORCPT ); Fri, 17 Jun 2016 05:02:19 -0400 Date: Fri, 17 Jun 2016 11:02:13 +0200 From: Ingo Molnar To: Kees Cook Cc: Thomas Garnier , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Borislav Petkov , Juergen Gross , Matt Fleming , Toshi Kani , Baoquan He , Andrew Morton , Dan Williams , Dave Hansen , "Aneesh Kumar K.V" , "Kirill A. Shutemov" , Martin Schwidefsky , Andy Lutomirski , Alexander Kuleshov , Alexander Popov , Joerg Roedel , Dave Young , Lv Zheng , Mark Salter , Stephen Smalley , Dmitry Vyukov , Boris Ostrovsky , David Rientjes , Christian Borntraeger , Jan Beulich , Kefeng Wang , Seth Jennings , Yinghai Lu , linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64) Message-ID: <20160617090213.GC4791@gmail.com> References: <1464217055-17654-1-git-send-email-keescook@chromium.org> <1464217055-17654-2-git-send-email-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1464217055-17654-2-git-send-email-keescook@chromium.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Kees Cook wrote: > From: Thomas Garnier > > Minor change that allows early boot physical mapping of PUD level virtual > addresses. The current implementation expects the virtual address to be > PUD aligned. For KASLR memory randomization, we need to be able to > randomize the offset used on the PUD table. > > It has no impact on current usage. > > Signed-off-by: Thomas Garnier > Signed-off-by: Kees Cook > --- > arch/x86/mm/init_64.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c > index bce2e5d9edd4..f205f39bd808 100644 > --- a/arch/x86/mm/init_64.c > +++ b/arch/x86/mm/init_64.c > @@ -454,10 +454,10 @@ phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end, > { > unsigned long pages = 0, next; > unsigned long last_map_addr = end; > - int i = pud_index(addr); > + int i = pud_index((unsigned long)__va(addr)); > > > for (; i < PTRS_PER_PUD; i++, addr = next) { > - pud_t *pud = pud_page + pud_index(addr); > + pud_t *pud = pud_page + pud_index((unsigned long)__va(addr)); > pmd_t *pmd; > pgprot_t prot = PAGE_KERNEL; So I really dislike two things about this code. Firstly a pre-existing problem is that the parameter names to phys_pud_init() suck: static unsigned long __meminit phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end, unsigned long page_size_mask) so 'unsigned long addr' is usually the signature of a virtual address - but that's no true here: it's a physical address. Same goes for 'unsigned long end'. Plus it's unclear what the connection between 'addr' and 'end' - it's not at all obvious 'at a glance' that they are the start and end addresses of a physical memory range. All of these problems can be solved by renaming them to 'paddr_start' and 'paddr_end'. Btw., I believe this misnomer and confusing code resulted in the buggy 'pud_index(addr)' not being noticed to begin with ... Secondly, and that's a new problem introduced by this patch: > + int i = pud_index((unsigned long)__va(addr)); > + pud_t *pud = pud_page + pud_index((unsigned long)__va(addr)); ... beyond the repetition, using type casts is fragile. Type casts should be a red flag to anyone involved in low level, security relevant code! So I'm pretty unhappy about seeing such a problem in such a patch. This code should be doing something like: unsigned long vaddr_start = __va(paddr_start); ... which gets rid of the type cast, the repetition and documents the code much better as well. Also see how easily the connection between the variables is self-documented just by picking names carefully: paddr_start paddr_end vaddr_start vaddr_end Also, _please_ add a comment to phys_pud_init() that explains what the function does. Thanks, Ingo