From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936319AbcJVASz (ORCPT ); Fri, 21 Oct 2016 20:18:55 -0400 Received: from mail-vk0-f42.google.com ([209.85.213.42]:35589 "EHLO mail-vk0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936138AbcJVASw (ORCPT ); Fri, 21 Oct 2016 20:18:52 -0400 MIME-Version: 1.0 In-Reply-To: <20161021123243.GC27807@codeblueprint.co.uk> References: <20161021123243.GC27807@codeblueprint.co.uk> From: Andy Lutomirski Date: Fri, 21 Oct 2016 17:18:30 -0700 Message-ID: Subject: Re: [tip:x86/asm] x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y) To: Matt Fleming Cc: "linux-efi@vger.kernel.org" , Brian Gerst , "linux-tip-commits@vger.kernel.org" , Thomas Gleixner , Ingo Molnar , Linus Torvalds , Ard Biesheuvel , Josh Poimboeuf , Denys Vlasenko , "H. Peter Anvin" , "linux-kernel@vger.kernel.org" , Nadav Amit , Borislav Petkov , Peter Zijlstra 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 Oct 21, 2016 5:32 AM, "Matt Fleming" wrote: > > On Wed, 24 Aug, at 06:03:04AM, tip-bot for Andy Lutomirski wrote: > > Commit-ID: e37e43a497d5a8b7c0cc1736d56986f432c394c9 > > Gitweb: http://git.kernel.org/tip/e37e43a497d5a8b7c0cc1736d56986f432c394c9 > > Author: Andy Lutomirski > > AuthorDate: Thu, 11 Aug 2016 02:35:23 -0700 > > Committer: Ingo Molnar > > CommitDate: Wed, 24 Aug 2016 12:11:42 +0200 > > > > x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y) > > > > This allows x86_64 kernels to enable vmapped stacks by setting > > HAVE_ARCH_VMAP_STACK=y - which enables the CONFIG_VMAP_STACK=y > > high level Kconfig option. > > > > There are a couple of interesting bits: > > This commit broke booting EFI mixed mode kernels. Here's what I've got > queued up to fix it. > > --- > From acf11e55bfcef7a1dca7d1735f4a780e0cdb1c89 Mon Sep 17 00:00:00 2001 > From: Matt Fleming > Date: Thu, 20 Oct 2016 22:17:21 +0100 > Subject: [PATCH] x86/efi: Prevent mixed mode boot corruption with > CONFIG_VMAP_STACK > > Booting an EFI mixed mode kernel has been crashing since commit: > > e37e43a497d5 ("x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y)") > > The user-visible effect in my test setup was the kernel being unable > to find the root file system ramdisk. This was likely caused by silent > memory or page table corruption. > > Enabling CONFIG_DEBUG_VIRTUAL immediately flagged the thunking code as > abusing virt_to_phys() because it was passing addresses that were not > part of the kernel direct mapping. > > Use the slow version instead, which correctly handles all memory > regions by performing a page table walk. > > Suggested-by: Andy Lutomirski > Cc: Ard Biesheuvel > Cc: Ingo Molnar > Cc: Thomas Gleixner > Cc: "H. Peter Anvin" > Signed-off-by: Matt Fleming > --- > arch/x86/platform/efi/efi_64.c | 59 +++++++++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index 58b0f801f66f..e3569a00885b 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -211,6 +211,17 @@ void efi_sync_low_kernel_mappings(void) > memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries); > } > > +/* > + * Wrapper for slow_virt_to_phys() that handles NULL addresses. > + */ > +static inline phys_addr_t virt_to_phys_or_null(void *va) > +{ > + if (!va) > + return 0; > + > + return slow_virt_to_phys(va); > +} > + This is asking for trouble if any of the variable length parameters are on the stack. How about adding a "size_t size" parameter and doing: if (!va) { return 0; } else if (virt_addr_valid(va)) { return virt_to_phys(va); } else { /* A fully aligned variable on the stack is guaranteed not to cross a page boundary. */ WARN_ON(!IS_ALIGNED((uintptr_t)va, size) || size > PAGE_SIZE); return slow_virt_to_phys(va); } From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Subject: Re: [tip:x86/asm] x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y) Date: Fri, 21 Oct 2016 17:18:30 -0700 Message-ID: References: <20161021123243.GC27807@codeblueprint.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20161021123243.GC27807-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Matt Fleming Cc: "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Brian Gerst , "linux-tip-commits-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Thomas Gleixner , Ingo Molnar , Linus Torvalds , Ard Biesheuvel , Josh Poimboeuf , Denys Vlasenko , "H. Peter Anvin" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Nadav Amit , Borislav Petkov , Peter Zijlstra List-Id: linux-efi@vger.kernel.org On Oct 21, 2016 5:32 AM, "Matt Fleming" wrote: > > On Wed, 24 Aug, at 06:03:04AM, tip-bot for Andy Lutomirski wrote: > > Commit-ID: e37e43a497d5a8b7c0cc1736d56986f432c394c9 > > Gitweb: http://git.kernel.org/tip/e37e43a497d5a8b7c0cc1736d56986f432c394c9 > > Author: Andy Lutomirski > > AuthorDate: Thu, 11 Aug 2016 02:35:23 -0700 > > Committer: Ingo Molnar > > CommitDate: Wed, 24 Aug 2016 12:11:42 +0200 > > > > x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y) > > > > This allows x86_64 kernels to enable vmapped stacks by setting > > HAVE_ARCH_VMAP_STACK=y - which enables the CONFIG_VMAP_STACK=y > > high level Kconfig option. > > > > There are a couple of interesting bits: > > This commit broke booting EFI mixed mode kernels. Here's what I've got > queued up to fix it. > > --- > From acf11e55bfcef7a1dca7d1735f4a780e0cdb1c89 Mon Sep 17 00:00:00 2001 > From: Matt Fleming > Date: Thu, 20 Oct 2016 22:17:21 +0100 > Subject: [PATCH] x86/efi: Prevent mixed mode boot corruption with > CONFIG_VMAP_STACK > > Booting an EFI mixed mode kernel has been crashing since commit: > > e37e43a497d5 ("x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y)") > > The user-visible effect in my test setup was the kernel being unable > to find the root file system ramdisk. This was likely caused by silent > memory or page table corruption. > > Enabling CONFIG_DEBUG_VIRTUAL immediately flagged the thunking code as > abusing virt_to_phys() because it was passing addresses that were not > part of the kernel direct mapping. > > Use the slow version instead, which correctly handles all memory > regions by performing a page table walk. > > Suggested-by: Andy Lutomirski > Cc: Ard Biesheuvel > Cc: Ingo Molnar > Cc: Thomas Gleixner > Cc: "H. Peter Anvin" > Signed-off-by: Matt Fleming > --- > arch/x86/platform/efi/efi_64.c | 59 +++++++++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index 58b0f801f66f..e3569a00885b 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -211,6 +211,17 @@ void efi_sync_low_kernel_mappings(void) > memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries); > } > > +/* > + * Wrapper for slow_virt_to_phys() that handles NULL addresses. > + */ > +static inline phys_addr_t virt_to_phys_or_null(void *va) > +{ > + if (!va) > + return 0; > + > + return slow_virt_to_phys(va); > +} > + This is asking for trouble if any of the variable length parameters are on the stack. How about adding a "size_t size" parameter and doing: if (!va) { return 0; } else if (virt_addr_valid(va)) { return virt_to_phys(va); } else { /* A fully aligned variable on the stack is guaranteed not to cross a page boundary. */ WARN_ON(!IS_ALIGNED((uintptr_t)va, size) || size > PAGE_SIZE); return slow_virt_to_phys(va); }