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

[David Edmondson <dme@dme.org>]

On Friday, 2021-04-23 at 17:37:53 GMT, Sean Christopherson wrote:

> 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?

Given that it doesn't prevent any known leakage, it's purely aesthetic,
which is why I'm happy not to bother.

> 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.

Perhaps an invalid instruction would be more useful in this respect, but
INT03 fills a similar purpose.

> 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.

Understood.

So is the conclusion that KVM should copy only insn_size bytes rather
than the full 15?

dme.
-- 
But they'll laugh at you in Jackson, and I'll be dancin' on a Pony Keg.