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

[Roman Kagan <rkagan@virtuozzo.com>]

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.