From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754517AbcFTQRU (ORCPT ); Mon, 20 Jun 2016 12:17:20 -0400 Received: from mail-yw0-f174.google.com ([209.85.161.174]:34124 "EHLO mail-yw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752226AbcFTQRH (ORCPT ); Mon, 20 Jun 2016 12:17:07 -0400 MIME-Version: 1.0 In-Reply-To: <20160617090213.GC4791@gmail.com> References: <1464217055-17654-1-git-send-email-keescook@chromium.org> <1464217055-17654-2-git-send-email-keescook@chromium.org> <20160617090213.GC4791@gmail.com> From: Thomas Garnier Date: Mon, 20 Jun 2016 09:17:05 -0700 Message-ID: Subject: Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64) To: Ingo Molnar Cc: Kees Cook , 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 , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jun 17, 2016 at 2:02 AM, Ingo Molnar wrote: > > * 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 ... > I will add a new commit that rename variables as described. > 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. Unfortunately, we can't do that because __va return a void*. We will get this warning on compile: arch/x86/mm/init_64.c:537:8: warning: assignment makes integer from pointer without a cast [enabled by default] vaddr = __va(paddr_start); If we used void*, we would need to type cast even more places. What do you think? > 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 > Will do on kernel_physical_mapping_init down. > Also, _please_ add a comment to phys_pud_init() that explains what the function > does. > Will do. > Thanks, > > Ingo