From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Garnier Subject: Re: [PATCH v2 3/3] x86: Make the GDT remapping read-only on 64 bit Date: Mon, 6 Feb 2017 14:10:36 -0800 Message-ID: References: <20170126165940.30799-1-thgarnie@google.com> <20170126165940.30799-3-thgarnie@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: List-Post: List-Help: List-Unsubscribe: List-Subscribe: In-Reply-To: To: Andy Lutomirski Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov , Kees Cook , Arjan van de Ven , Paul Gortmaker , Borislav Petkov , "Rafael J . Wysocki" , Len Brown , Pavel Machek , Jiri Kosina , Matt Fleming , Ard Biesheuvel , Boris Ostrovsky , Juergen Gross , Rusty Russell , Christian Borntraeger , Fenghua Yu , He Chen , Brian Gerst , Luis List-Id: linux-efi@vger.kernel.org On Wed, Feb 1, 2017 at 9:14 PM, Andy Lutomirski wrote: > On Thu, Jan 26, 2017 at 8:59 AM, Thomas Garnier wro= te: >> This patch makes the GDT remapped pages read-only to prevent corruption. >> This change is done only on 64 bit. >> >> The native_load_tr_desc function was adapted to correctly handle a >> read-only GDT. The LTR instruction always writes to the GDT TSS entry. >> This generates a page fault if the GDT is read-only. This change checks >> if the current GDT is a remap and swap GDTs as needed. This function was >> tested by booting multiple machines and checking hibernation works >> properly. >> >> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the >> per-cpu variable was removed for functions to fetch the original GDT. >> Instead of reloading the previous GDT, VMX will reload the fixmap GDT as >> expected. For testing, VMs were started and restored on multiple >> configurations. >> >> Signed-off-by: Thomas Garnier >> --- >> Based on next-20170125 >> --- >> arch/x86/include/asm/desc.h | 46 +++++++++++++++++++++++++++++++++= ++----- >> arch/x86/include/asm/processor.h | 1 + >> arch/x86/kernel/cpu/common.c | 28 ++++++++++++++++++------ >> arch/x86/kvm/svm.c | 4 +--- >> arch/x86/kvm/vmx.c | 15 +++++-------- >> 5 files changed, 70 insertions(+), 24 deletions(-) >> >> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h >> index 4cc176f57b78..ca7b2224fcb4 100644 >> --- a/arch/x86/include/asm/desc.h >> +++ b/arch/x86/include/asm/desc.h >> @@ -52,6 +52,12 @@ static inline struct desc_struct *get_cpu_direct_gdt(= unsigned int cpu) >> return per_cpu(gdt_page, cpu).gdt; >> } >> >> +/* Provide the current original GDT */ >> +static inline struct desc_struct *get_current_direct_gdt(void) >> +{ >> + return this_cpu_ptr(&gdt_page)->gdt; >> +} > > I'm assuming that the reason that this isn't part of patch 2 and used > instead of the version that takes cpu as a parameter is that TLS > doesn't work until the GDT is set up. If so, perhaps that's worthy of > a comment in patch 2. > > But give this_cpu_read(gdt_page.gdt) a try, please. > I tried but I can't get it working properly because the gdt field is an array, not a pointer. For example with this_cpu_read(gdt_page.gdt), I get: ./arch/x86/include/asm/desc.h: In function =E2=80=98get_current_gdt_rw=E2= =80=99: ./include/linux/percpu-defs.h:308:21: error: incompatible types when assigning to type =E2=80=98struct desc_struct[16]=E2=80=99 from type =E2=80= =98struct desc_struct *=E2=80=99 case 1: pscr_ret__ =3D stem##1(variable); break; \ ^ I tried different variants without success. What do you think? >> +/* >> + * The LTR instruction marks the TSS GDT entry as busy. In 64bit, the G= DT is >> + * a read-only remapping. To prevent a page fault, the GDT is switched = to the >> + * original writeable version when needed. >> + */ >> +#ifdef CONFIG_X86_64 >> +static inline void native_load_tr_desc(void) >> +{ >> + struct desc_ptr gdt; >> + int cpu =3D raw_smp_processor_id(); >> + bool restore =3D false; >> + struct desc_struct *fixmap_gdt; >> + >> + native_store_gdt(&gdt); > > Off the top of my head, this is something like 10 cycles. IMO that's > fast enough not to worry about the regression this will cause to KVM > exits. In any event, we'll get that back and *much* more when we do > the optimizations that this series enables. --=20 Thomas From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 In-Reply-To: References: <20170126165940.30799-1-thgarnie@google.com> <20170126165940.30799-3-thgarnie@google.com> From: Thomas Garnier Date: Mon, 6 Feb 2017 14:10:36 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [kernel-hardening] Re: [PATCH v2 3/3] x86: Make the GDT remapping read-only on 64 bit To: Andy Lutomirski Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov , Kees Cook , Arjan van de Ven , Paul Gortmaker , Borislav Petkov , "Rafael J . Wysocki" , Len Brown , Pavel Machek , Jiri Kosina , Matt Fleming , Ard Biesheuvel , Boris Ostrovsky , Juergen Gross , Rusty Russell , Christian Borntraeger , Fenghua Yu , He Chen , Brian Gerst , "Luis R . Rodriguez" , Adam Buchbinder , Stanislaw Gruszka , Arnd Bergmann , Dave Hansen , Chen Yucong , Vitaly Kuznetsov , David Vrabel , Josh Poimboeuf , Tim Chen , Rik van Riel , Andi Kleen , Jiri Olsa , Prarit Bhargava , Michael Ellerman , Joerg Roedel , Paolo Bonzini , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , X86 ML , "linux-kernel@vger.kernel.org" , kasan-dev , "linux-pm@vger.kernel.org" , "linux-efi@vger.kernel.org" , "xen-devel@lists.xenproject.org" , lguest@lists.ozlabs.org, kvm list , "kernel-hardening@lists.openwall.com" List-ID: On Wed, Feb 1, 2017 at 9:14 PM, Andy Lutomirski wrote: > On Thu, Jan 26, 2017 at 8:59 AM, Thomas Garnier wro= te: >> This patch makes the GDT remapped pages read-only to prevent corruption. >> This change is done only on 64 bit. >> >> The native_load_tr_desc function was adapted to correctly handle a >> read-only GDT. The LTR instruction always writes to the GDT TSS entry. >> This generates a page fault if the GDT is read-only. This change checks >> if the current GDT is a remap and swap GDTs as needed. This function was >> tested by booting multiple machines and checking hibernation works >> properly. >> >> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the >> per-cpu variable was removed for functions to fetch the original GDT. >> Instead of reloading the previous GDT, VMX will reload the fixmap GDT as >> expected. For testing, VMs were started and restored on multiple >> configurations. >> >> Signed-off-by: Thomas Garnier >> --- >> Based on next-20170125 >> --- >> arch/x86/include/asm/desc.h | 46 +++++++++++++++++++++++++++++++++= ++----- >> arch/x86/include/asm/processor.h | 1 + >> arch/x86/kernel/cpu/common.c | 28 ++++++++++++++++++------ >> arch/x86/kvm/svm.c | 4 +--- >> arch/x86/kvm/vmx.c | 15 +++++-------- >> 5 files changed, 70 insertions(+), 24 deletions(-) >> >> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h >> index 4cc176f57b78..ca7b2224fcb4 100644 >> --- a/arch/x86/include/asm/desc.h >> +++ b/arch/x86/include/asm/desc.h >> @@ -52,6 +52,12 @@ static inline struct desc_struct *get_cpu_direct_gdt(= unsigned int cpu) >> return per_cpu(gdt_page, cpu).gdt; >> } >> >> +/* Provide the current original GDT */ >> +static inline struct desc_struct *get_current_direct_gdt(void) >> +{ >> + return this_cpu_ptr(&gdt_page)->gdt; >> +} > > I'm assuming that the reason that this isn't part of patch 2 and used > instead of the version that takes cpu as a parameter is that TLS > doesn't work until the GDT is set up. If so, perhaps that's worthy of > a comment in patch 2. > > But give this_cpu_read(gdt_page.gdt) a try, please. > I tried but I can't get it working properly because the gdt field is an array, not a pointer. For example with this_cpu_read(gdt_page.gdt), I get: ./arch/x86/include/asm/desc.h: In function =E2=80=98get_current_gdt_rw=E2= =80=99: ./include/linux/percpu-defs.h:308:21: error: incompatible types when assigning to type =E2=80=98struct desc_struct[16]=E2=80=99 from type =E2=80= =98struct desc_struct *=E2=80=99 case 1: pscr_ret__ =3D stem##1(variable); break; \ ^ I tried different variants without success. What do you think? >> +/* >> + * The LTR instruction marks the TSS GDT entry as busy. In 64bit, the G= DT is >> + * a read-only remapping. To prevent a page fault, the GDT is switched = to the >> + * original writeable version when needed. >> + */ >> +#ifdef CONFIG_X86_64 >> +static inline void native_load_tr_desc(void) >> +{ >> + struct desc_ptr gdt; >> + int cpu =3D raw_smp_processor_id(); >> + bool restore =3D false; >> + struct desc_struct *fixmap_gdt; >> + >> + native_store_gdt(&gdt); > > Off the top of my head, this is something like 10 cycles. IMO that's > fast enough not to worry about the regression this will cause to KVM > exits. In any event, we'll get that back and *much* more when we do > the optimizations that this series enables. --=20 Thomas