Re: [PATCH v4 10/15] KVM: SVM: Introduce helper functions to (de)activate AVIC and x2AVIC

[Maxim Levitsky]

On Wed, 2022-05-11 at 22:37 +0700, Suravee Suthikulpanit wrote:
> Maxim,
> 
> On 5/9/22 8:42 PM, Maxim Levitsky wrote:
> > ...
> > 
> > So I did some testing, and reviewed this code again with regard to nesting,
> > and now I see that it has CVE worthy bug, so have to revoke my Reviewed-By.
> > 
> > This is what happens:
> > 
> > On nested VM entry, *request to inhibit AVIC is done*, and then nested msr bitmap
> > is calculated, still with all X2AVIC msrs open,
> > 
> > 1. nested_svm_vmrun -> enter_svm_guest_mode -> kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > 2. nested_svm_vmrun -> nested_svm_vmrun_msrpm
> > 
> > But the nested guest will be entered without AVIC active
> > (since we don't yet support nested avic and it is optional anyway), thus if the nested guest
> > also doesn't intercept those msrs, it will gain access to the *host* x2apic msrs. Ooops.
> 
> Shouldn't this be changed to intercept the x2APIC msrs because of the following logic?
> 
> kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu)
>      kvm_vcpu_update_apicv(vcpu)
>          static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu)
>              avic_deactivate_vmcb()
>                  svm_set_x2apic_msr_interception(true)

Nope because the above only updates L1 msr intercept bitmap, while 'merged'
msr bitmap that L2 uses still has those msrs open.

Other and better way to fix it would be to fix set_msr_interception
to update the merged bitmap as well.

I think I will post a patch series to clean up this mess soon.

Best regards,
	Maxim Levitsky

> 
> > I think the easist way to fix this for now, is to make nested_svm_vmrun_msrpm
> > never open access to x2apic msrs regardless of the host bitmap value, but in the long
> > term the whole thing needs to be refactored.
> 
> Agree.
> 
> > Another thing I noted is that avic_deactivate_vmcb should not touch avic msrs
> > when avic_mode == AVIC_MODE_X1, it is just a waste of time.
> 
> We can add the check.
> 
> > Also updating these msr intercepts is pointless if the guest doesn't use x2apic.
> 
> We can also add the check.
> 
> Best Regards,
> Suravee
>