On Fri, Apr 23, 2021, David Edmondson wrote: > On Friday, 2021-04-23 at 15:33:47 GMT, Sean Christopherson wrote: > > > On Thu, Apr 22, 2021, David Edmondson wrote: > >> 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. Yes, so what's the point of clearing the unused bytes? Doing so won't magically fix a broken userspace. That's why I argue that 0x90 or 0xcc would be more appropriate; there's at least a non-zero chance that it will help userspace avoid doing something completely broken. On the other hand, userspace can guard against a broken _KVM_ by initializing vcpu->run with a known pattern and logging if KVM exits to userspace with seemingly bogus data. Crushing the unused bytes to zero defeats userspace's sanity check, e.g. if the actual memcpy() of the instruction bytes copies the wrong number of bytes, then userspace's magic pattern will be lost and debugging the KVM bug will be that much harder. This is very much not a theoretical problem, I have debugged two separate KVM bugs in the last few months where KVM completely failed to set vcpu->run->exit_reason before exiting to userspace. The exit_reason is a bit of a special case because it's disturbingly easy for KVM to get confused over return values and unintentionally exit to userspace, but it's not a big stretch to imagine a bug where KVM provides incomplete data.
