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

[Sean Christopherson <seanjc@google.com>]

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