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.
