On Friday, 2021-04-23 at 17:37:53 GMT, Sean Christopherson wrote: > 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? Given that it doesn't prevent any known leakage, it's purely aesthetic, which is why I'm happy not to bother. > 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. Perhaps an invalid instruction would be more useful in this respect, but INT03 fills a similar purpose. > 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. Understood. So is the conclusion that KVM should copy only insn_size bytes rather than the full 15? dme. -- But they'll laugh at you in Jackson, and I'll be dancin' on a Pony Keg.
