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

[Sean Christopherson <seanjc@google.com>]

On Fri, Mar 13, 2026, Yosry Ahmed wrote:
> > @@ -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. 

No, because the intercept has higher priority than the #GP due to bad RAX.

> My initial instincth 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.

Except for the erratum case.

> The actual problem is that the current code does not check if L1
> actually sets the intercept in emulate_svm_instr(). 

Oh dagnabbit.  I had thought about this, multiple times, but wrote it off as a
non-issue because if L1 wanted to intercept VMWHATEVER, KVM would set the intercept
in vmcb02 and would get _that_ instead of a #GP.  But the erratum case means that
hardware could have signaled #GP even when the instruction should have been
intercepted.

And I also forgot the KVM could be intercepting #GP for the VMware crud, which
would unintentionally grab the CPL case too.  Darn kitchen sink #GPs.

> 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;

Don't you need the page_address_valid() check here?  Ooooh, no, because either
emulate_svm_instr() will synthesize #VMEXIT, or svm_instr_handlers() will take
care of the #GP.  It's only CPL that needs to be checked early, because it has
priority over the #VMEXIT.

>                 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?

Go ahead and send an entirely new series.  The less threads I have to chase down
after I get back, the less likely I am to screw things up :-)