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

[Yang Zhang <yang.zhang.wz@gmail.com>]

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