Re: [PATCH] x86/vmx: Revert "x86/VMX: sanitize rIP before re-entering guest"

[Jan Beulich <jbeulich@suse.com>]

On 09.10.2020 17:09, Andrew Cooper wrote:
> At the time of XSA-170, the x86 instruction emulator really was broken, and
> would allow arbitrary non-canonical values to be loaded into %rip.  This was
> fixed after the embargo by c/s 81d3a0b26c1 "x86emul: limit-check branch
> targets".
> 
> However, in a demonstration that off-by-one errors really are one of the
> hardest programming issues we face, everyone involved with XSA-170, myself
> included, mistook the statement in the SDM which says:
> 
>   If the processor supports N < 64 linear-address bits, bits 63:N must be identical
> 
> to mean "must be canonical".  A real canonical check is bits 63:N-1.
> 
> VMEntries really do tolerate a not-quite-canonical %rip, specifically to cater
> to the boundary condition at 0x0000800000000000.
> 
> Now that the emulator has been fixed, revert the XSA-170 change to fix
> architectural behaviour at the boundary case.  The XTF test case for XSA-170
> exercises this corner case, and still passes.
> 
> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

But why revert the change rather than fix ...

> @@ -4280,38 +4280,6 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>  out:
>      if ( nestedhvm_vcpu_in_guestmode(v) )
>          nvmx_idtv_handling();
> -
> -    /*
> -     * VM entry will fail (causing the guest to get crashed) if rIP (and
> -     * rFLAGS, but we don't have an issue there) doesn't meet certain
> -     * criteria. As we must not allow less than fully privileged mode to have
> -     * such an effect on the domain, we correct rIP in that case (accepting
> -     * this not being architecturally correct behavior, as the injected #GP
> -     * fault will then not see the correct [invalid] return address).
> -     * And since we know the guest will crash, we crash it right away if it
> -     * already is in most privileged mode.
> -     */
> -    mode = vmx_guest_x86_mode(v);
> -    if ( mode == 8 ? !is_canonical_address(regs->rip)

... the wrong use of is_canonical_address() here? By reverting
you open up avenues for XSAs in case we get things wrong elsewhere,
including ...

> -                   : regs->rip != regs->eip )

... for 32-bit guests.

Jan