Re: [PATCH v3 3/7] KVM: SVM: Move RAX legality check to SVM insn interception handlers

[Yosry Ahmed]

> @@ -2306,24 +2312,18 @@ static int gp_interception(struct kvm_vcpu *vcpu)
>                 goto reinject;
>
>         opcode = svm_instr_opcode(vcpu);
> +       if (opcode != NONE_SVM_INSTR)
> +               return emulate_svm_instr(vcpu, opcode);
>
> -       if (opcode == NONE_SVM_INSTR) {
> -               if (!enable_vmware_backdoor)
> -                       goto reinject;
> -
> -               /*
> -                * VMware backdoor emulation on #GP interception only handles
> -                * IN{S}, OUT{S}, and RDPMC.
> -                */
> -               if (!is_guest_mode(vcpu))
> -                       return kvm_emulate_instruction(vcpu,
> -                               EMULTYPE_VMWARE_GP | EMULTYPE_NO_DECODE);
> -       } else {
> -               if (!page_address_valid(vcpu, svm->vmcb->save.rax))
> -                       goto reinject;
> +       if (!enable_vmware_backdoor)
> +               goto reinject;
>
> -               return emulate_svm_instr(vcpu, opcode);
> -       }
> +       /*
> +        * VMware backdoor emulation on #GP interception only handles
> +        * IN{S}, OUT{S}, and RDPMC.
> +        */
> +       if (!is_guest_mode(vcpu))
> +               return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP | EMULTYPE_NO_DECODE);

AI review pointed out that we should not drop the page_address_valid()
from here, because if an SVM instruction is executed by L2, and KVM
intercepts the #GP, it should re-inject the #GP into L2 if RAX is
illegal instead of synthesizing a #VMEXIT to L1. My initial instinct
is to keep the check here as well as in the intercept handlers, but
no, L1's intercept should take precedence over #GP due to invalid RAX
anyway. In fact, if L1 has the intercept set, then it must be set in
vmcb02, and KVM would get a #VMEXIT on the intercept not on #GP.

The actual problem is that the current code does not check if L1
actually sets the intercept in emulate_svm_instr(). So if L1 and KVM
do not set the intercept, and RAX is invalid, the current code could
synthesize a spurious #VMEXIT to L1 instead of reinjecting #GP. The
existing check on RAX prevents that, but it doesn't really fix the
problem because if we get #GP due to CPL != 0, we'll still generate a
spurious #VMEXIT to L1. What we really should be doing in
gp_interception() is:

1. if CPL != 0, re-inject #GP.
2. If in guest mode and L1 intercepts the instruction, synthesize a #VMEXIT.
3. Otherwise emulate the instruction, which would take care of
re-injecting the #GP if RAX is invalid with this patch.

Something like this on top (over 2 patches):

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cf5ebdc4b27bf..8942272eb80b2 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2237,10 +2237,11 @@ static int emulate_svm_instr(struct kvm_vcpu
*vcpu, int opcode)
                [SVM_INSTR_VMLOAD] = vmload_interception,
                [SVM_INSTR_VMSAVE] = vmsave_interception,
        };
+       int exit_code = guest_mode_exit_codes[opcode];
        struct vcpu_svm *svm = to_svm(vcpu);

-       if (is_guest_mode(vcpu)) {
-               nested_svm_simple_vmexit(svm, guest_mode_exit_codes[opcode]);
+       if (is_guest_mode(vcpu) &&
vmcb12_is_intercept(&svm->nested.ctl, exit_code))
+               nested_svm_simple_vmexit(svm, exit_code);
                return 1;
        }
        return svm_instr_handlers[opcode](vcpu);
@@ -2269,8 +2270,11 @@ static int gp_interception(struct kvm_vcpu *vcpu)
                goto reinject;

        opcode = svm_instr_opcode(vcpu);
-       if (opcode != NONE_SVM_INSTR)
+       if (opcode != NONE_SVM_INSTR) {
+               if (svm->vmcb->save.cpl)
+                       goto reinject;
                return emulate_svm_instr(vcpu, opcode);
+       }

        if (!enable_vmware_backdoor)
                goto reinject;

---

Sean, do you prefer that I send patches separately on top of this
series or a new version with these patches included?