[PATCH v2] kvm:vmx: more complete state update on APICv on/off

[PATCH v2] kvm:vmx: more complete state update on APICv on/off

[Roman Kagan]

The function to update APICv on/off state (in particular, to deactivate
it when enabling Hyper-V SynIC), used to be incomplete: it didn't adjust
APICv-related fields among secondary processor-based VM-execution
controls.

As a result, Windows 2012 guests would get stuck when SynIC-based
auto-EOI interrupt intersected with e.g. an IPI in the guest.

In addition, the MSR intercept bitmap wasn't updated to correspond to
whether "virtualize x2APIC mode" was enabled. [This path used to be
untriggerable, since Windows didn't use x2APIC but rather their own
synthetic APIC access MSRs.]

The patch fixes those omissions.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: Steve Rutherford <srutherford@google.com>
---
v1 -> v2:
 - only update relevant bits in the secondary exec control
 - update msr intercept bitmap (also make x2apic msr bitmap always
   correspond to APICv)

 arch/x86/kvm/vmx.c | 47 +++++++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ee1c8a9..5f9701e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2418,7 +2418,8 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
 
 	if (is_guest_mode(vcpu))
 		msr_bitmap = vmx_msr_bitmap_nested;
-	else if (vcpu->arch.apic_base & X2APIC_ENABLE) {
+	else if (vcpu->arch.apic_base & X2APIC_ENABLE &&
+		 kvm_vcpu_apicv_active(vcpu)) {
 		if (is_long_mode(vcpu))
 			msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
 		else
@@ -4783,6 +4784,19 @@ static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
+	if (cpu_has_secondary_exec_ctrls()) {
+		if (kvm_vcpu_apicv_active(vcpu))
+			vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
+				      SECONDARY_EXEC_APIC_REGISTER_VIRT |
+				      SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
+		else
+			vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
+					SECONDARY_EXEC_APIC_REGISTER_VIRT |
+					SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
+	}
+
+	if (cpu_has_vmx_msr_bitmap())
+		vmx_set_msr_bitmap(vcpu);
 }
 
 static u32 vmx_exec_control(struct vcpu_vmx *vmx)
@@ -6329,23 +6343,20 @@ static __init int hardware_setup(void)
 
 	set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
 
-	if (enable_apicv) {
-		for (msr = 0x800; msr <= 0x8ff; msr++)
-			vmx_disable_intercept_msr_read_x2apic(msr);
-
-		/* According SDM, in x2apic mode, the whole id reg is used.
-		 * But in KVM, it only use the highest eight bits. Need to
-		 * intercept it */
-		vmx_enable_intercept_msr_read_x2apic(0x802);
-		/* TMCCT */
-		vmx_enable_intercept_msr_read_x2apic(0x839);
-		/* TPR */
-		vmx_disable_intercept_msr_write_x2apic(0x808);
-		/* EOI */
-		vmx_disable_intercept_msr_write_x2apic(0x80b);
-		/* SELF-IPI */
-		vmx_disable_intercept_msr_write_x2apic(0x83f);
-	}
+	for (msr = 0x800; msr <= 0x8ff; msr++)
+		vmx_disable_intercept_msr_read_x2apic(msr);
+
+	/* According SDM, in x2apic mode, the whole id reg is used.  But in
+	 * KVM, it only use the highest eight bits. Need to intercept it */
+	vmx_enable_intercept_msr_read_x2apic(0x802);
+	/* TMCCT */
+	vmx_enable_intercept_msr_read_x2apic(0x839);
+	/* TPR */
+	vmx_disable_intercept_msr_write_x2apic(0x808);
+	/* EOI */
+	vmx_disable_intercept_msr_write_x2apic(0x80b);
+	/* SELF-IPI */
+	vmx_disable_intercept_msr_write_x2apic(0x83f);
 
 	if (enable_ept) {
 		kvm_mmu_set_mask_ptes(0ull,
-- 
2.5.5

Re: [PATCH v2] kvm:vmx: more complete state update on APICv on/off

[Yang Zhang]

On 2016/5/17 18:36, Roman Kagan wrote:

During reviewing this patch, I think there is an secure issue in 
upstream KVM since the 5c919412fe61c:
1. The enable_apicv is setting to 1 if the underling hardware has APICv 
capability.
2. set the x2apic_read/write msr to disable the interception for most 
x2apic read and TPR/EOI/SELF-IPI write.
3. vmx_set_virtual_x2apic_mode is called when guest enables the 
x2apic,but kvm_vcpu_apicv_active() check may fail if synIc is 
activated.This means the virtualize_x2apic_mode will not set even guest 
enabling the x2apic.
4. In the following vmx_set_msr_bitmap, it will use the x2apic msr 
bitmap without check whether virtualize_x2apic_mode is enabled and apicv 
is enabled.
5. Then the access to the msr that doesn't intercept by hypervisor will 
access the hardware directly.


> The function to update APICv on/off state (in particular, to deactivate
> it when enabling Hyper-V SynIC), used to be incomplete: it didn't adjust
> APICv-related fields among secondary processor-based VM-execution
> controls.
>
> As a result, Windows 2012 guests would get stuck when SynIC-based
> auto-EOI interrupt intersected with e.g. an IPI in the guest.
>
> In addition, the MSR intercept bitmap wasn't updated to correspond to
> whether "virtualize x2APIC mode" was enabled. [This path used to be
> untriggerable, since Windows didn't use x2APIC but rather their own
> synthetic APIC access MSRs.]
>
> The patch fixes those omissions.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Steve Rutherford <srutherford@google.com>
> ---
> v1 -> v2:
>   - only update relevant bits in the secondary exec control
>   - update msr intercept bitmap (also make x2apic msr bitmap always
>     correspond to APICv)
>
>   arch/x86/kvm/vmx.c | 47 +++++++++++++++++++++++++++++------------------
>   1 file changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee1c8a9..5f9701e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2418,7 +2418,8 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
>
>   	if (is_guest_mode(vcpu))
>   		msr_bitmap = vmx_msr_bitmap_nested;
> -	else if (vcpu->arch.apic_base & X2APIC_ENABLE) {
> +	else if (vcpu->arch.apic_base & X2APIC_ENABLE &&
> +		 kvm_vcpu_apicv_active(vcpu)) {

Need to check the virtualize_x2apic_mode is enabled or not.

>   		if (is_long_mode(vcpu))
>   			msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
>   		else
> @@ -4783,6 +4784,19 @@ static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>
>   	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
> +	if (cpu_has_secondary_exec_ctrls()) {
> +		if (kvm_vcpu_apicv_active(vcpu))
> +			vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> +				      SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +				      SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +		else
> +			vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> +					SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +					SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +	}
> +
> +	if (cpu_has_vmx_msr_bitmap())
> +		vmx_set_msr_bitmap(vcpu);
>   }
>
>   static u32 vmx_exec_control(struct vcpu_vmx *vmx)
> @@ -6329,23 +6343,20 @@ static __init int hardware_setup(void)
>
>   	set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
>
> -	if (enable_apicv) {
> -		for (msr = 0x800; msr <= 0x8ff; msr++)
> -			vmx_disable_intercept_msr_read_x2apic(msr);
> -
> -		/* According SDM, in x2apic mode, the whole id reg is used.
> -		 * But in KVM, it only use the highest eight bits. Need to
> -		 * intercept it */
> -		vmx_enable_intercept_msr_read_x2apic(0x802);
> -		/* TMCCT */
> -		vmx_enable_intercept_msr_read_x2apic(0x839);
> -		/* TPR */
> -		vmx_disable_intercept_msr_write_x2apic(0x808);
> -		/* EOI */
> -		vmx_disable_intercept_msr_write_x2apic(0x80b);
> -		/* SELF-IPI */
> -		vmx_disable_intercept_msr_write_x2apic(0x83f);
> -	}
> +	for (msr = 0x800; msr <= 0x8ff; msr++)
> +		vmx_disable_intercept_msr_read_x2apic(msr);
> +
> +	/* According SDM, in x2apic mode, the whole id reg is used.  But in
> +	 * KVM, it only use the highest eight bits. Need to intercept it */
> +	vmx_enable_intercept_msr_read_x2apic(0x802);
> +	/* TMCCT */
> +	vmx_enable_intercept_msr_read_x2apic(0x839);
> +	/* TPR */
> +	vmx_disable_intercept_msr_write_x2apic(0x808);
> +	/* EOI */
> +	vmx_disable_intercept_msr_write_x2apic(0x80b);
> +	/* SELF-IPI */
> +	vmx_disable_intercept_msr_write_x2apic(0x83f);
>
>   	if (enable_ept) {
>   		kvm_mmu_set_mask_ptes(0ull,
>


-- 
best regards
yang

Re: [PATCH v2] kvm:vmx: more complete state update on APICv on/off

[Roman Kagan]

On Wed, May 18, 2016 at 01:24:50PM +0800, Yang Zhang wrote:
> On 2016/5/17 18:36, Roman Kagan wrote:
> 
> During reviewing this patch, I think there is an secure issue in upstream
> KVM since the 5c919412fe61c:
> 1. The enable_apicv is setting to 1 if the underling hardware has APICv
> capability.
> 2. set the x2apic_read/write msr to disable the interception for most x2apic
> read and TPR/EOI/SELF-IPI write.
> 3. vmx_set_virtual_x2apic_mode is called when guest enables the x2apic,but
> kvm_vcpu_apicv_active() check may fail if synIc is activated.This means the
> virtualize_x2apic_mode will not set even guest enabling the x2apic.
> 4. In the following vmx_set_msr_bitmap, it will use the x2apic msr bitmap
> without check whether virtualize_x2apic_mode is enabled and apicv is
> enabled.
> 5. Then the access to the msr that doesn't intercept by hypervisor will
> access the hardware directly.

Looks like a valid concern, thanks.

> > @@ -2418,7 +2418,8 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
> > 
> >   	if (is_guest_mode(vcpu))
> >   		msr_bitmap = vmx_msr_bitmap_nested;
> > -	else if (vcpu->arch.apic_base & X2APIC_ENABLE) {
> > +	else if (vcpu->arch.apic_base & X2APIC_ENABLE &&
> > +		 kvm_vcpu_apicv_active(vcpu)) {
> 
> Need to check the virtualize_x2apic_mode is enabled or not.

Indeed.  Have lost it while porting over from our branch; will resubmit.
Thanks!

Roman.