Re: [PATCH v2 1/2] kvm: x86: Allow userspace to handle emulation errors

[Sean Christopherson <seanjc@google.com>]

On Fri, Apr 23, 2021, David Edmondson wrote:
> On Friday, 2021-04-23 at 15:33:47 GMT, Sean Christopherson wrote:
> 
> > On Thu, Apr 22, 2021, David Edmondson wrote:
> >> Agreed. As Jim indicated in his other reply, there should be no new data
> >> leaked by not zeroing the bytes.
> >> 
> >> For now at least, this is not a performance critical path, so clearing
> >> the payload doesn't seem too onerous.
> >
> > I feel quite strongly that KVM should _not_ touch the unused bytes.
> 
> I'm fine with that, but...
> 
> > As Jim pointed out, a stream of 0x0 0x0 0x0 ... is not benign, it will
> > decode to one or more ADD instructions.  Arguably 0x90, 0xcc, or an
> > undending stream of prefixes would be more appropriate so that it's
> > less likely for userspace to decode a bogus instruction.
> 
> ...I don't understand this position. If the user-level instruction
> decoder starts interpreting bytes that the kernel did *not* indicate as
> valid (by setting insn_size to include them), it's broken.

Yes, so what's the point of clearing the unused bytes?  Doing so won't magically
fix a broken userspace.  That's why I argue that 0x90 or 0xcc would be more
appropriate; there's at least a non-zero chance that it will help userspace avoid
doing something completely broken.

On the other hand, userspace can guard against a broken _KVM_ by initializing
vcpu->run with a known pattern and logging if KVM exits to userspace with
seemingly bogus data.  Crushing the unused bytes to zero defeats userspace's
sanity check, e.g. if the actual memcpy() of the instruction bytes copies the
wrong number of bytes, then userspace's magic pattern will be lost and debugging
the KVM bug will be that much harder.

This is very much not a theoretical problem, I have debugged two separate KVM
bugs in the last few months where KVM completely failed to set
vcpu->run->exit_reason before exiting to userspace.  The exit_reason is a bit of
a special case because it's disturbingly easy for KVM to get confused over return
values and unintentionally exit to userspace, but it's not a big stretch to
imagine a bug where KVM provides incomplete data.