| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 26 May 2017 18:05:51 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Borislav Petkov <bpetkov@...e.de>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Mel Gorman <mgorman@...e.de>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
Nadav Amit <nadav.amit@...il.com>,
Radim Krčmář <rkrcmar@...hat.com>,
kvm@...r.kernel.org, Rik van Riel <riel@...hat.com>,
Dave Hansen <dave.hansen@...el.com>,
Nadav Amit <namit@...are.com>, Michal Hocko <mhocko@...e.com>,
Arjan van de Ven <arjan@...ux.intel.com>
Subject: Re: [PATCH v3 8/8] x86,kvm: Teach KVM's VMX code that CR3 isn't a
constant
On 26/05/2017 17:30, Paolo Bonzini wrote:
>
>
> On 26/05/2017 02:47, Andy Lutomirski wrote:
>> When PCID is enabled, CR3's PCID bits can change during context
>> switches, so KVM won't be able to treat CR3 as a per-mm constant any
>> more.
>>
>> I structured this like the existing CR4 handling. Under ordinary
>> circumstances (PCID disabled or if the current PCID and the value
>> that's already in the VMCS match), then we won't do an extra VMCS
>> write, and we'll never do an extra direct CR3 read. The overhead
>> should be minimal.
>>
>> I disallowed using the new helper in non-atomic context because
>> PCID support will cause CR3 to stop being constant in non-atomic
>> process context.
>>
>> (Frankly, it also scares me a bit that KVM ever treated CR3 as
>> constant, but it looks like it was okay before.)
>>
>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>> Cc: kvm@...r.kernel.org
>> Cc: Rik van Riel <riel@...hat.com>
>> Cc: Dave Hansen <dave.hansen@...el.com>
>> Cc: Nadav Amit <namit@...are.com>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Arjan van de Ven <arjan@...ux.intel.com>
>> Signed-off-by: Andy Lutomirski <luto@...nel.org>
>> ---
>> arch/x86/include/asm/mmu_context.h | 19 +++++++++++++++++++
>> arch/x86/kvm/vmx.c | 21 ++++++++++++++++++---
>> 2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index 187c39470a0b..f20d7ea47095 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -266,4 +266,23 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
>> return __pkru_allows_pkey(vma_pkey(vma), write);
>> }
>>
>> +
>> +/*
>> + * This can be used from process context to figure out what the value of
>> + * CR3 is without needing to do a (slow) read_cr3().
>> + *
>> + * It's intended to be used for code like KVM that sneakily changes CR3
>> + * and needs to restore it. It needs to be used very carefully.
>> + */
>> +static inline unsigned long __get_current_cr3_fast(void)
>> +{
>> + unsigned long cr3 = __pa(this_cpu_read(cpu_tlbstate.loaded_mm)->pgd);
>> +
>> + /* For now, be very restrictive about when this can be called. */
>> + VM_WARN_ON(in_nmi() || !in_atomic());
>> +
>> + VM_BUG_ON(cr3 != read_cr3());
>> + return cr3;
>> +}
>> +
>> #endif /* _ASM_X86_MMU_CONTEXT_H */
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 72f78396bc09..b7b36c9ffa3d 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -48,6 +48,7 @@
>> #include <asm/kexec.h>
>> #include <asm/apic.h>
>> #include <asm/irq_remapping.h>
>> +#include <asm/mmu_context.h>
>>
>> #include "trace.h"
>> #include "pmu.h"
>> @@ -596,6 +597,7 @@ struct vcpu_vmx {
>> int gs_ldt_reload_needed;
>> int fs_reload_needed;
>> u64 msr_host_bndcfgs;
>> + unsigned long vmcs_host_cr3; /* May not match real cr3 */
>> unsigned long vmcs_host_cr4; /* May not match real cr4 */
>> } host_state;
>> struct {
>> @@ -5012,12 +5014,19 @@ static void vmx_set_constant_host_state(struct vcpu_vmx *vmx)
>> u32 low32, high32;
>> unsigned long tmpl;
>> struct desc_ptr dt;
>> - unsigned long cr0, cr4;
>> + unsigned long cr0, cr3, cr4;
>>
>> cr0 = read_cr0();
>> WARN_ON(cr0 & X86_CR0_TS);
>> vmcs_writel(HOST_CR0, cr0); /* 22.2.3 */
>> - vmcs_writel(HOST_CR3, read_cr3()); /* 22.2.3 FIXME: shadow tables */
>> +
>> + /*
>> + * Save the most likely value for this task's CR3 in the VMCS.
>> + * We can't use __get_current_cr3_fast() because we're not atomic.
>> + */
>> + cr3 = read_cr3();
>> + vmcs_writel(HOST_CR3, cr3); /* 22.2.3 FIXME: shadow tables */
>> + vmx->host_state.vmcs_host_cr3 = cr3;
>>
>> /* Save the most likely value for this task's CR4 in the VMCS. */
>> cr4 = cr4_read_shadow();
>> @@ -8843,7 +8852,7 @@ static void vmx_arm_hv_timer(struct kvm_vcpu *vcpu)
>> static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>> {
>> struct vcpu_vmx *vmx = to_vmx(vcpu);
>> - unsigned long debugctlmsr, cr4;
>> + unsigned long debugctlmsr, cr3, cr4;
>>
>> /* Don't enter VMX if guest state is invalid, let the exit handler
>> start emulation until we arrive back to a valid state */
>> @@ -8865,6 +8874,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>> if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))
>> vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
>>
>> + cr3 = __get_current_cr3_fast();
>> + if (unlikely(cr3 != vmx->host_state.vmcs_host_cr3)) {
>> + vmcs_writel(HOST_CR3, cr3);
>> + vmx->host_state.vmcs_host_cr3 = cr3;
>> + }
>> +
>> cr4 = cr4_read_shadow();
>> if (unlikely(cr4 != vmx->host_state.vmcs_host_cr4)) {
>> vmcs_writel(HOST_CR4, cr4);
>>
>
> Queued, thanks. If anybody needs a topic branch, please holler.
Ah, no, it depends on the others. Note to self, compile first, answer
second.
Paolo
Powered by blists - more mailing lists