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

[David Edmondson <dme@dme.org>]

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.

>> Footnotes:
>> [1]  https://disaster-area.hh.sledj.net/tmp/dme-581090/
>>
>> dme.
>> --
>> Music has magic, it's good clear syncopation.

dme.
-- 
Don't you know you're never going to get to France.