On Fri, Apr 23, 2021 at 10:57 AM Jim Mattson <jmattson@xxxxxxxxxx> wrote: > > 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. Oh, or if the CS limit is reached. (cf. AMD's APM, volume 2, section 15.8.4: Nested and intercepted #PF). > > dme. > > -- > > But they'll laugh at you in Jackson, and I'll be dancin' on a Pony Keg.
