On Fri, Apr 23, 2021 at 10:55 AM David Edmondson <dme@xxxxxxx> wrote: > > 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? Insn_size should almost always be 15. It will only be less when the emulator hits a page crossing before fetching 15 bytes and it can't fetch from the second page. > dme. > -- > But they'll laugh at you in Jackson, and I'll be dancin' on a Pony Keg.
