| Message ID | 20190912235603.18954-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Handle unexpected MMIO accesses using master abort semantics | expand |
On Thu, Sep 12, 2019 at 04:56:03PM -0700, Sean Christopherson wrote: > Use master abort semantics, i.e. reads return all ones and writes are > dropped, to handle unexpected MMIO accesses when reading guest memory > instead of returning X86EMUL_IO_NEEDED, which in turn gets interpreted > as a guest page fault. > > Emulation of certain instructions, notably VMX instructions, involves > reading or writing guest memory without going through the emulator. > These emulation flows are not equipped to handle MMIO accesses as no > sane and properly functioning guest kernel will target MMIO with such > instructions, and so simply inject a page fault in response to > X86EMUL_IO_NEEDED. > > While not 100% correct, using master abort semantics is at least > sometimes correct, e.g. non-existent MMIO accesses do actually master > abort, whereas injecting a page fault is always wrong, i.e. the issue > lies in the physical address domain, not in the virtual to physical > translation. > > Apply the logic to kvm_write_guest_virt_system() in addition to > replacing existing #PF logic in kvm_read_guest_virt(), as VMPTRST uses > the former, i.e. can also leak a host stack address. > > Reported-by: Fuqian Huang <huangfq.daxian@gmail.com> > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/x86.c | 40 +++++++++++++++++++++++++++++++--------- > 1 file changed, 31 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b4cfd786d0b6..d1d7e9fac17a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5234,16 +5234,24 @@ int kvm_read_guest_virt(struct kvm_vcpu *vcpu, > struct x86_exception *exception) > { > u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0; > + int r; > + > + r = kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access, > + exception); > > /* > - * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED > - * is returned, but our callers are not ready for that and they blindly > - * call kvm_inject_page_fault. Ensure that they at least do not leak > - * uninitialized kernel stack memory into cr2 and error code. > + * FIXME: this should technically call out to userspace to handle the > + * MMIO access, but our callers are not ready for that, so emulate > + * master abort behavior instead, i.e. writes are dropped. Dagnabbit, fixed this to make it 'reads return all ones' and forgot to commit.. v2 on its way. > */ > - memset(exception, 0, sizeof(*exception)); > - return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access, > - exception); > + if (r == X86EMUL_IO_NEEDED) { > + memset(val, 0xff, bytes); > + return 0; > + } > + if (r == X86EMUL_PROPAGATE_FAULT) > + return -EFAULT; > + WARN_ON_ONCE(r); > + return 0; > } > EXPORT_SYMBOL_GPL(kvm_read_guest_virt); > > @@ -5317,11 +5325,25 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v > int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val, > unsigned int bytes, struct x86_exception *exception) > { > + int r; > + > /* kvm_write_guest_virt_system can pull in tons of pages. */ > vcpu->arch.l1tf_flush_l1d = true; > > - return kvm_write_guest_virt_helper(addr, val, bytes, vcpu, > - PFERR_WRITE_MASK, exception); > + r = kvm_write_guest_virt_helper(addr, val, bytes, vcpu, > + PFERR_WRITE_MASK, exception); > + > + /* > + * FIXME: this should technically call out to userspace to handle the > + * MMIO access, but our callers are not ready for that, so emulate > + * master abort behavior instead, i.e. writes are dropped. > + */ > + if (r == X86EMUL_IO_NEEDED) > + return 0; > + if (r == X86EMUL_PROPAGATE_FAULT) > + return -EFAULT; > + WARN_ON_ONCE(r); > + return 0; > } > EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system); > > -- > 2.22.0 >
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b4cfd786d0b6..d1d7e9fac17a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5234,16 +5234,24 @@ int kvm_read_guest_virt(struct kvm_vcpu *vcpu, struct x86_exception *exception) { u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0; + int r; + + r = kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access, + exception); /* - * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED - * is returned, but our callers are not ready for that and they blindly - * call kvm_inject_page_fault. Ensure that they at least do not leak - * uninitialized kernel stack memory into cr2 and error code. + * FIXME: this should technically call out to userspace to handle the + * MMIO access, but our callers are not ready for that, so emulate + * master abort behavior instead, i.e. writes are dropped. */ - memset(exception, 0, sizeof(*exception)); - return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access, - exception); + if (r == X86EMUL_IO_NEEDED) { + memset(val, 0xff, bytes); + return 0; + } + if (r == X86EMUL_PROPAGATE_FAULT) + return -EFAULT; + WARN_ON_ONCE(r); + return 0; } EXPORT_SYMBOL_GPL(kvm_read_guest_virt); @@ -5317,11 +5325,25 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val, unsigned int bytes, struct x86_exception *exception) { + int r; + /* kvm_write_guest_virt_system can pull in tons of pages. */ vcpu->arch.l1tf_flush_l1d = true; - return kvm_write_guest_virt_helper(addr, val, bytes, vcpu, - PFERR_WRITE_MASK, exception); + r = kvm_write_guest_virt_helper(addr, val, bytes, vcpu, + PFERR_WRITE_MASK, exception); + + /* + * FIXME: this should technically call out to userspace to handle the + * MMIO access, but our callers are not ready for that, so emulate + * master abort behavior instead, i.e. writes are dropped. + */ + if (r == X86EMUL_IO_NEEDED) + return 0; + if (r == X86EMUL_PROPAGATE_FAULT) + return -EFAULT; + WARN_ON_ONCE(r); + return 0; } EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
Use master abort semantics, i.e. reads return all ones and writes are dropped, to handle unexpected MMIO accesses when reading guest memory instead of returning X86EMUL_IO_NEEDED, which in turn gets interpreted as a guest page fault. Emulation of certain instructions, notably VMX instructions, involves reading or writing guest memory without going through the emulator. These emulation flows are not equipped to handle MMIO accesses as no sane and properly functioning guest kernel will target MMIO with such instructions, and so simply inject a page fault in response to X86EMUL_IO_NEEDED. While not 100% correct, using master abort semantics is at least sometimes correct, e.g. non-existent MMIO accesses do actually master abort, whereas injecting a page fault is always wrong, i.e. the issue lies in the physical address domain, not in the virtual to physical translation. Apply the logic to kvm_write_guest_virt_system() in addition to replacing existing #PF logic in kvm_read_guest_virt(), as VMPTRST uses the former, i.e. can also leak a host stack address. Reported-by: Fuqian Huang <huangfq.daxian@gmail.com> Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/x86.c | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-)