From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2 1/4] KVM: x86: Validate guest writes to MSR_IA32_APICBASE Date: Fri, 24 Jan 2014 17:01:00 +0100 Message-ID: <52E28E3C.8070104@redhat.com> References: <1ba67ddad48673feb70c7c3a8bd0b62cef9d89cb.1390578527.git.jan.kiszka@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm To: Jan Kiszka , Gleb Natapov , Marcelo Tosatti Return-path: Received: from mail-ea0-f178.google.com ([209.85.215.178]:47716 "EHLO mail-ea0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751282AbaAXQBH (ORCPT ); Fri, 24 Jan 2014 11:01:07 -0500 Received: by mail-ea0-f178.google.com with SMTP id a15so1077486eae.23 for ; Fri, 24 Jan 2014 08:01:05 -0800 (PST) In-Reply-To: <1ba67ddad48673feb70c7c3a8bd0b62cef9d89cb.1390578527.git.jan.kiszka@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 24/01/2014 16:48, Jan Kiszka ha scritto: > Check for invalid state transitions on guest-initiated updates of > MSR_IA32_APICBASE. This address both enabling of the x2APIC when it is > not supported and all invalid transitions as described in SDM section > 10.12.5. It also checks that no reserved bit is set in APICBASE by the > guest. > > Signed-off-by: Jan Kiszka > --- > arch/x86/kvm/cpuid.h | 16 ++++++++++++++++ > arch/x86/kvm/lapic.h | 2 +- > arch/x86/kvm/vmx.c | 9 +++++---- > arch/x86/kvm/x86.c | 32 +++++++++++++++++++++++++------- > 4 files changed, 47 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index f1e4895..b012ad2 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -72,4 +72,20 @@ static inline bool guest_cpuid_has_pcid(struct kvm_vcpu *vcpu) > return best && (best->ecx & bit(X86_FEATURE_PCID)); > } > > +static inline bool guest_cpuid_has_x2apic(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpuid_entry2 *best; > + > + best = kvm_find_cpuid_entry(vcpu, 1, 0); > + return best && (best->ecx & bit(X86_FEATURE_X2APIC)); > +} > + > +static inline unsigned int guest_cpuid_get_phys_bits(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpuid_entry2 *best; > + > + best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0); > + return best ? best->eax & 0xff : 36; > +} [Resending after learning that Ctrl-Shift-C does other things beyond copying to clipboard] There's already cpuid_maxphyaddr for this. I can adjust it when committing. This is applied to kvm/queue. The other three will have to wait for after the merge window. Paolo > #endif > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index c8b0d0d..6a11845 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -65,7 +65,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, > struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map); > > u64 kvm_get_apic_base(struct kvm_vcpu *vcpu); > -void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data); > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info); > void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu, > struct kvm_lapic_state *s); > int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu); > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 5c88791..a06f101 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -4392,7 +4392,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx) > static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > - u64 msr; > + struct msr_data apic_base_msr; > > vmx->rmode.vm86_active = 0; > > @@ -4400,10 +4400,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) > > vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); > kvm_set_cr8(&vmx->vcpu, 0); > - msr = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; > + apic_base_msr.data = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; > if (kvm_vcpu_is_bsp(&vmx->vcpu)) > - msr |= MSR_IA32_APICBASE_BSP; > - kvm_set_apic_base(&vmx->vcpu, msr); > + apic_base_msr.data |= MSR_IA32_APICBASE_BSP; > + apic_base_msr.host_initiated = true; > + kvm_set_apic_base(&vmx->vcpu, &apic_base_msr); > > vmx_segment_cache_clear(vmx); > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 0c76f7c..f4b0591 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -257,10 +257,26 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu) > } > EXPORT_SYMBOL_GPL(kvm_get_apic_base); > > -void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data) > -{ > - /* TODO: reserve bits check */ > - kvm_lapic_set_base(vcpu, data); > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > +{ > + u64 old_state = vcpu->arch.apic_base & > + (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE); > + u64 new_state = msr_info->data & > + (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE); > + u64 reserved_bits = ((~0ULL) << guest_cpuid_get_phys_bits(vcpu)) | > + 0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE); > + > + if (!msr_info->host_initiated && > + ((msr_info->data & reserved_bits) != 0 || > + new_state == X2APIC_ENABLE || > + (new_state == MSR_IA32_APICBASE_ENABLE && > + old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) || > + (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) && > + old_state == 0))) > + return 1; > + > + kvm_lapic_set_base(vcpu, msr_info->data); > + return 0; > } > EXPORT_SYMBOL_GPL(kvm_set_apic_base); > > @@ -2006,8 +2022,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case 0x200 ... 0x2ff: > return set_msr_mtrr(vcpu, msr, data); > case MSR_IA32_APICBASE: > - kvm_set_apic_base(vcpu, data); > - break; > + return kvm_set_apic_base(vcpu, msr_info); > case APIC_BASE_MSR ... APIC_BASE_MSR + 0x3ff: > return kvm_x2apic_msr_write(vcpu, msr, data); > case MSR_IA32_TSCDEADLINE: > @@ -6409,6 +6424,7 @@ EXPORT_SYMBOL_GPL(kvm_task_switch); > int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > struct kvm_sregs *sregs) > { > + struct msr_data apic_base_msr; > int mmu_reset_needed = 0; > int pending_vec, max_bits, idx; > struct desc_ptr dt; > @@ -6432,7 +6448,9 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > > mmu_reset_needed |= vcpu->arch.efer != sregs->efer; > kvm_x86_ops->set_efer(vcpu, sregs->efer); > - kvm_set_apic_base(vcpu, sregs->apic_base); > + apic_base_msr.data = sregs->apic_base; > + apic_base_msr.host_initiated = true; > + kvm_set_apic_base(vcpu, &apic_base_msr); > > mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0; > kvm_x86_ops->set_cr0(vcpu, sregs->cr0); >