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.
