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

[David Edmondson <dme@dme.org>]

On Wednesday, 2021-04-21 at 09:24:13 -07, Aaron Lewis wrote:

>> > +     if (insn_size) {
>> > +             run->emulation_failure.ndata = 3;
>> > +             run->emulation_failure.flags |=
>> > +                     KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
>> > +             run->emulation_failure.insn_size = insn_size;
>> > +             memcpy(run->emulation_failure.insn_bytes,
>> > +                    ctxt->fetch.data, sizeof(ctxt->fetch.data));
>>
>> We're relying on the fact that insn_bytes is at least as large as
>> fetch.data, which is fine, but worth an assertion?
>>
>> "Leaking" irrelevant bytes here also seems bad, but I can't immediately
>> see a problem as a result.
>>
>
> 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().

Footnotes:
[1]  https://disaster-area.hh.sledj.net/tmp/dme-581090/

dme.
-- 
Music has magic, it's good clear syncopation.