Re: [PATCH v2 1/6] KVM: SVM: Use maxphyaddr in emulator RAX check for VMRUN/VMLOAD/VMSAVE

[Yosry Ahmed]

> > > Honestly, why bother keeping check_svme_pa()?  Can't we just do the checks in
> > > svm_check_intercept()?  E.g. vmx_check_intercept() already "injects" #UD for RDTSCP.
> >
> > Hmm svm_check_intercept() isn't semantically the right place AFAICT,
>
> Actually, I think it is.  Per the APM:
>
>   Generally, instruction intercepts are checked after simple exceptions (such as
>   #GP—when CPL is incorrect—or #UD) have been checked, but before exceptions
>   related to memory accesses (such as page faults) and exceptions based on
>   specific operand values.
>
> This #GP is operand specific.

Yes, but from KVM's perspective, svm_check_intercept() is meant to
check if the instruction executed by L2 needs to be intercepted by L1.
What this patch is doing is not specific to instructions executed by
L2.

Anyway, that's irrelevant now (see below).

>
> > and more importantly, it's only called if the instruction is executed
> > in guest mode (i.e. in L2+).
>
> Hold up, we're getting ahead of ourselves.
>
> The only legitimate reason the emulator is at all aware of VMSAVE, VMLOAD, and
> VMRUN is to deal with #GP due to the RAX check, because hardware checks the GPA
> against the host's physical address space.  See commit 82a11e9c6fa2 ("KVM: SVM:
> Add emulation support for #GP triggered by SVM instructions").
>
> The emulator "support" was originally added by commit 01de8b09e606 ("KVM: SVM:
> Add intercept checks for SVM instructions"), but AFAICT, for all intents and
> purposes that was dead code when it was added, because the emulator doesn't
> actually _emulate_ the instructions.  I assume if they aren't intercepted, and
> KVM is full on emulating instead of just decoding, they end up at EMULATION_FAILED
> and get a #UD or something.
>
> Outside of forced emulation or code stream rewriting, KVM should _never_ fully
> emulate any of the SVM instructions except VMMCALL (and that is a super special
> case).  KVM does need to _decode_ the instruction, and it needs to get the
> pre-intercept exception checks correct so that KVM correctly injects e.g. #GP
> instead of synthesizing a #VMEXIT for the CPL check, but KVM doesn't need to do
> *all* of the checks.
>
> Note, for L2, the SVME check is meaningless, as EFER.SVME has to be set for L2
> to be active, i.e. it's L1's responsibility to handle that check.
>
> Back to the physical address thing, KVM _already_ handles that check in the #GP
> path,

I guess if KVM is not intercepting #GP, then the hardware injects the
#GP and the emulator still doesn't have to worry about it -- because
we don't support the case where RAX can be legal from the host's
perspective but not the guest's. Makes sense.

> it's just wrong too:
>
>                 /* All SVM instructions expect page aligned RAX */
>                 if (svm->vmcb->save.rax & ~PAGE_MASK)
>                         goto reinject;
>
> So I think what we want is to
>
>   (a) fix the RAX check in gp_interception()
>   (b) drop the RAX check in the emulator
>   (c) add a CPL check in the emulator (because the intercepted #GP could have
>       been due to L2 executing at CPL>0, not due to a bad-but-good RAX).

Agree with this (also thanks for pointing out page_address_valid()).
Will add 3 patches doing this at the beginning of the series instead
of this one.