Re: [PATCH v2 1/2] KVM: SVM: Triple fault L1 on unintercepted EFER.SVME clear by L2

[Yosry Ahmed]

> > What if we key off vcpu->wants_to_run?
>
> That crossed my mind too.
>
> > It's less protection against false positives from things like
> > kvm_vcpu_reset() if it didn't leave nested before clearing EFER, but
> > more protection against the #VMEXIT case you mentioned. Also should be
> > much lower on the fugliness scale imo.
>
> Yeah, I had pretty much the exact same thought process and assessment.  I suggested
> the WRMSR approach because I'm not sure how I feel about using wants_to_run for
> functional behavior.  But after realizing that hooking WRMSR won't handle RSM,
> I'm solidly against my WRMSR idea.
>
> Honestly, I'm leaning slightly towards dropping this patch entirely since it's
> not a bug fix.  But I'm definitely not completely against it either.  So what if
> we throw it in, but plan on reverting if there are any more problems (that aren't
> obviously due to goofs elsewhere in KVM).

I am okay with that.

>
> Is this what you were thinking?

Yeah, exactly.

>
> ---
>  arch/x86/kvm/svm/svm.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 1b31b033d79b..3e48e9c1c955 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -216,6 +216,19 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>
>         if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
>                 if (!(efer & EFER_SVME)) {
> +                       /*
> +                        * Architecturally, clearing EFER.SVME while a guest is
> +                        * running yields undefined behavior, i.e. KVM can do
> +                        * literally anything.  Force the vCPU back into L1 as
> +                        * that is the safest option for KVM, but synthesize a
> +                        * triple fault (for L1!) so that KVM at least doesn't
> +                        * run random L2 code in the context of L1.  Do so if
> +                        * and only if the vCPU is actively running, e.g. to
> +                        * avoid positives if userspace is stuffing state.
> +                        */
> +                       if (is_guest_mode(vcpu) && vcpu->wants_to_run)
> +                               kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> +
>                         svm_leave_nested(vcpu);
>                         /* #GP intercept is still needed for vmware backdoor */
>                         if (!enable_vmware_backdoor)
>
> base-commit: 95deaec3557dced322e2540bfa426e60e5373d46
> --