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

[David Edmondson <dme@dme.org>]

On Friday, 2021-04-23 at 15:33:47 GMT, Sean Christopherson wrote:

> On Thu, Apr 22, 2021, David Edmondson wrote:
>> On Wednesday, 2021-04-21 at 12:01:21 -07, Aaron Lewis wrote:
>> 
>> >> >
>> >> > I don't think this is a problem because the instruction bytes stream
>> >> > has irrelevant bytes in it anyway.  In the test attached I verify that
>> >> > it receives an flds instruction in userspace that was emulated in the
>> >> > guest.  In the stream that comes through insn_size is set to 15 and
>> >> > the instruction is only 2 bytes long, so the stream has irrelevant
>> >> > bytes in it as far as this instruction is concerned.
>> >>
>> >> As an experiment I added[1] reporting of the exit reason using flag 2. On
>> >> emulation failure (without the instruction bytes flag enabled), one run
>> >> of QEMU reported:
>> >>
>> >> > KVM internal error. Suberror: 1
>> >> > extra data[0]: 2
>> >> > extra data[1]: 4
>> >> > extra data[2]: 0
>> >> > extra data[3]: 31
>> >> > emulation failure
>> >>
>> >> data[1] and data[2] are not indicated as valid, but it seems unfortunate
>> >> that I got (not really random) garbage there.
>> >>
>> >> Admittedly, with only your patches applied ndata will never skip past
>> >> any bytes, as there is only one flag. As soon as I add another, is it my
>> >> job to zero out those unused bytes? Maybe we should be clearing all of
>> >> the payload at the top of prepare_emulation_failure_exit().
>> >>
>> >
>> > Clearing the bytes at the top of prepare_emulation_failure_exit()
>> > sounds good to me.  That will keep the data more deterministic.
>> > Though, I will say that I don't think that is required.  If the first
>> > flag isn't set the data shouldn't be read, no?
>> 
>> 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.

> I don't see any reason why unused insn bytes should be treated any differently
> than unused mmio.data[], or unused internal.data[], etc... 
>
> IMO, the better option is to do nothing and let userspace initialize vcpu->run
> before KVM_RUN if they want to avoid consuming stale data.  

dme.
-- 
I've still got sand in my shoes.