On Mon, Mar 11, 2019 at 8:01 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> Most (all?) x86 platforms provide a port IO based reset mechanism, e.g.
> OUT 92h or CF9h. Userspace may emulate said mechanism, i.e. reset a
> vCPU in response to KVM_EXIT_IO, without explicitly announcing to KVM
> that it is doing a reset, e.g. Qemu jams vCPU state and resumes running.
>
> To avoid corruping %rip after such a reset, commit 0967b7bf1c22 ("KVM:
> Skip pio instruction when it is emulated, not executed") changed the
> behavior of PIO handlers, i.e. today's "fast" PIO handling to skip the
> instruction prior to exiting to userspace. Full emulation doesn't need
> such tricks becase re-emulating the instruction will naturally handle
> %rip being changed to point at the reset vector.
>
> Updating %rip prior to executing to userspace has several drawbacks:
>
> - Userspace sees the wrong %rip on the exit, e.g. if PIO emulation
> fails it will likely yell about the wrong address.
> - Single step exits to userspace for are effectively dropped as
> KVM_EXIT_DEBUG is overwritten with KVM_EXIT_IO.
> - Behavior of PIO emulation is different depending on whether it
> goes down the fast path or the slow path.
>
> Rather than skip the PIO instruction before exiting to userspace,
> snapshot the linear %rip and cancel PIO completion if the current
> value does not match the snapshot. For a 64-bit vCPU, i.e. the most
> common scenario, the snapshot and comparison has negligible overhead
> as VMCS.GUEST_RIP will be cached regardless, i.e. there is no extra
> VMREAD in this case.
>
> All other alternatives to snapshotting the linear %rip that don't
> rely on an explicit reset announcenment suffer from one corner case
> or another. For example, canceling PIO completion on any write to
> %rip fails if userspace does a save/restore of %rip, and attempting to
> avoid that issue by canceling PIO only if %rip changed then fails if PIO
> collides with the reset %rip. Attempting to zero in on the exact reset
> vector won't work for APs, which means adding more hooks such as the
> vCPU's MP_STATE, and so on and so forth.
>
> Checking for a linear %rip match technically suffers from corner cases,
> e.g. userspace could theoretically rewrite the underlying code page and
> expect a different instruction to execute, or the guest hardcodes a PIO
> reset at 0xfffffff0, but those are far, far outside of what can be
> considered normal operation.
>
> Fixes: 432baf60eee3 ("KVM: VMX: use kvm_fast_pio_in for handling IN I/O")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Reported-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
>
> Although technically the "buggy" behavior goes back 10+ years, I used
> the recent VMX change for Fixes since that was the commit that actually
> led to a complaint. Arguably the commit that re-introduced fast IN for
> SVM (8370c3d08bd9 "kvm: svm: Add kvm_fast_pio_in support") should be
> blamed, but given that his is more along the lines of "that's weird" as
> opposed to "the world is burning", err on the side of cuation.
>
> That being said, odds are good that userspace won't even exercise the
> rip checks. Qemu has intentionally re-entered KVM to complete I/O since
> commit 9ccfac9ea4 ("kvm: Unconditionally reenter kernel after IO exits")
> in early 2011, i.e. testing this required modifying Qemu to not re-enter
> the kernel. And AFIAK no other userspace emulates port-based resets.
Does this break tools/testing/selftests/kvm/x86_64/state_test.c? IIRC,
that test assumes that the %rip is advanced early, since it performs a
migration with PIO completion pending. (Note that the documentation
seems to imply that this migration may be ill-advised, since the guest
state isn't consistent at this point.)
