From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752434AbdEQCzb (ORCPT ); Tue, 16 May 2017 22:55:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58294 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751360AbdEQCz2 (ORCPT ); Tue, 16 May 2017 22:55:28 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 480DF3B709 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=dyoung@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 480DF3B709 Date: Wed, 17 May 2017 10:55:13 +0800 From: Dave Young To: Baoquan He Cc: linux-kernel@vger.kernel.org, bp@alien8.de, bhsharma@redhat.com, rja@hpe.com, Matt Fleming , Ard Biesheuvel , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Thomas Garnier , Kees Cook , x86@kernel.org, linux-efi@vger.kernel.org Subject: Re: [PATCH v3] x86/efi: Correct ident mapping of efi old_map when kalsr enabled Message-ID: <20170517025513.GA9988@dhcp-128-65.nay.redhat.com> References: <1494647799-20600-1-git-send-email-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1494647799-20600-1-git-send-email-bhe@redhat.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 17 May 2017 02:55:27 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Baoquan On 05/13/17 at 11:56am, Baoquan He wrote: > For EFI with 'efi=old_map' kernel option specified, Kernel will panic > when kaslr is enabled. > > The back trace is: > > BUG: unable to handle kernel paging request at 000000007febd57e > IP: 0x7febd57e > PGD 1025a067 > PUD 0 > > Oops: 0010 [#1] SMP > [ ... ] > Call Trace: > ? efi_call+0x58/0x90 > ? printk+0x58/0x6f > efi_enter_virtual_mode+0x3c5/0x50d > start_kernel+0x40f/0x4b8 > ? set_init_arg+0x55/0x55 > ? early_idt_handler_array+0x120/0x120 > x86_64_start_reservations+0x24/0x26 > x86_64_start_kernel+0x14c/0x16f > start_cpu+0x14/0x14 > > The root cause is the ident mapping is not built correctly in old_map case. > > For nokaslr kernel, PAGE_OFFSET is 0xffff880000000000 which is PGDIR_SIZE > aligned. We can borrow the pud table from direct mapping safely. Given a > physical address X, we have pud_index(X) == pud_index(__va(X)). However, > for kaslr kernel, PAGE_OFFSET is PUD_SIZE aligned. For a given physical > address X, pud_index(X) != pud_index(__va(X)). We can't only copy pgd entry > from direct mapping to build ident mapping, instead need copy pud entry > one by one from direct mapping. > > Fix it. > > Signed-off-by: Baoquan He > Signed-off-by: Dave Young > Cc: Matt Fleming > Cc: Ard Biesheuvel > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Thomas Garnier > Cc: Kees Cook > Cc: x86@kernel.org > Cc: linux-efi@vger.kernel.org > --- > v2->v3: > 1. Rewrite code to copy pud entry one by one so that code can be understood > better. Usually we only have less than 1TB or several TB memory, pud entry > copy one by one won't impact efficiency. > > 2. Adding p4d page table handling. > > v1->v2: > Change code and add description according to Thomas's suggestion as below: > > 1. Add checking if pud table is allocated successfully. If not just break > the for loop. > > 2. Add code comment to explain how the 1:1 mapping is built in efi_call_phys_prolog > > 3. Other minor change > > arch/x86/platform/efi/efi_64.c | 69 +++++++++++++++++++++++++++++++++++++----- > 1 file changed, 61 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index c488625..c9dffec 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -71,11 +71,13 @@ static void __init early_code_mapping_set_exec(int executable) > > pgd_t * __init efi_call_phys_prolog(void) > { > - unsigned long vaddress; > - pgd_t *save_pgd; > + unsigned long vaddr, addr_pgd, addr_p4d, addr_pud; > + pgd_t *save_pgd, *pgd_k, *pgd_efi; > + p4d_t *p4d, *p4d_k, *p4d_efi; > + pud_t *pud; > > int pgd; > - int n_pgds; > + int n_pgds, i, j; > > if (!efi_enabled(EFI_OLD_MEMMAP)) { > save_pgd = (pgd_t *)read_cr3(); > @@ -88,10 +90,44 @@ pgd_t * __init efi_call_phys_prolog(void) > n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE); > save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL); > > + /* > + * Build 1:1 ident mapping for old_map usage. It needs to be noticed > + * that PAGE_OFFSET is PGDIR_SIZE aligned with KASLR disabled, while > + * PUD_SIZE ALIGNED with KASLR enabled. So for a given physical > + * address X, the pud_index(X) != pud_index(__va(X)), we can only copy > + * pud entry of __va(X) to fill in pud entry of X to build 1:1 mapping > + * . Means here we can only reuse pmd table of direct mapping. > + */ > for (pgd = 0; pgd < n_pgds; pgd++) { > - save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE); > - vaddress = (unsigned long)__va(pgd * PGDIR_SIZE); > - set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), *pgd_offset_k(vaddress)); > + addr_pgd = (unsigned long)(pgd * PGDIR_SIZE); > + vaddr = (unsigned long)__va(pgd * PGDIR_SIZE); > + pgd_efi = pgd_offset_k(addr_pgd); > + save_pgd[pgd] = *pgd_efi; > + p4d = p4d_alloc(&init_mm, pgd_efi, addr_pgd); > + > + if (!p4d) { > + pr_err("Failed to allocate p4d table \n"); > + goto out; > + } > + for(i=0; i + addr_p4d = addr_pgd + i * P4D_SIZE; > + p4d_efi = p4d + p4d_index(addr_p4d); > + pud = pud_alloc(&init_mm, p4d_efi, addr_p4d); > + if (!pud) { > + pr_err("Failed to allocate pud table \n"); > + goto out; > + } > + for(j=0; j + addr_pud = addr_p4d + j * PUD_SIZE; > + if (addr_pud > (max_pfn << PAGE_SHIFT)) > + break; > + vaddr = (unsigned long)__va(addr_pud); > + > + pgd_k = pgd_offset_k(vaddr); > + p4d_k = p4d_offset(pgd_k, vaddr); > + pud[j] = *pud_offset(p4d_k, vaddr); > + } > + } > } > out: > __flush_tlb_all(); > @@ -104,8 +140,11 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd) > /* > * After the lock is released, the original page table is restored. > */ > - int pgd_idx; > + int pgd_idx, i; > int nr_pgds; > + pgd_t *pgd; > + p4d_t *p4d; > + pud_t *pud; > > if (!efi_enabled(EFI_OLD_MEMMAP)) { > write_cr3((unsigned long)save_pgd); > @@ -115,9 +154,23 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd) > > nr_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE); > > - for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++) > + for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++) { > + pgd = pgd_offset_k(pgd_idx * PGDIR_SIZE); > set_pgd(pgd_offset_k(pgd_idx * PGDIR_SIZE), save_pgd[pgd_idx]); > > + if (!(pgd_val(*pgd) & _PAGE_PRESENT)) > + continue; > + for(i=0; i + p4d = p4d_offset(pgd, pgd_idx * PGDIR_SIZE + i * P4D_SIZE); > + if (!(p4d_val(*p4d) & _PAGE_PRESENT)) > + continue; > + pud = (pud_t*)p4d_page_vaddr(*p4d); > + pud_free(&init_mm, pud); > + } > + p4d = (p4d_t*)pgd_page_vaddr(*pgd); > + p4d_free(&init_mm, p4d); > + } > + > kfree(save_pgd); > > __flush_tlb_all(); > -- > 2.5.5 > Thanks Dave