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

[Yosry Ahmed]

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

No, this is wrong.. well it's incomplete. So we do need to check the
intercept in vmcb12, but, if it's not set, we'll end up with KVM
emulating the instructions through vmload_vmsave_interception(), and
treating RAX as an L1 GPA.

If L1 has VLS enabled though, this is wrong. KVM should treat RAX as an
L2 GPA an run it through the NPT first before using it (e.g. through
translate_nested_gpa()).

Synthesizing a spurious #VMEXIT(VMLOAD/VMSAVE) is definitely better than
letting L2 bypass L1's NPTs and access its memory. So this change is a
net loss. I will drop it from the next version, and this spurious
#VMEXIT can be fixed separately to keep this series focused on fixing
the non-architectural #GPs.

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