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

[Suravee Suthikulpanit]

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)

> 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