On 22/11/2016 20:21, Radim Krčmář wrote: > em_jmp_far and em_ret_far assumed that setting IP can only fail in 64 > bit mode, but syzkaller proved otherwise (and SDM agrees). > Code segment was restored upon failure, but it was left uninitialized > outside of long mode, which could lead to a leak of host kernel stack. > > Found by syzkaller: > > WARNING: CPU: 2 PID: 3668 at arch/x86/kvm/emulate.c:2217 > em_ret_far+0x428/0x480 > Kernel panic - not syncing: panic_on_warn set ... > > CPU: 2 PID: 3668 Comm: syz-executor Not tainted 4.9.0-rc4+ #49 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > [...] > Call Trace: > [...] __dump_stack lib/dump_stack.c:15 > [...] dump_stack+0xb3/0x118 lib/dump_stack.c:51 > [...] panic+0x1b7/0x3a3 kernel/panic.c:179 > [...] __warn+0x1c4/0x1e0 kernel/panic.c:542 > [...] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585 > [...] em_ret_far+0x428/0x480 arch/x86/kvm/emulate.c:2217 > [...] em_ret_far_imm+0x17/0x70 arch/x86/kvm/emulate.c:2227 > [...] x86_emulate_insn+0x87a/0x3730 arch/x86/kvm/emulate.c:5294 > [...] x86_emulate_instruction+0x520/0x1ba0 arch/x86/kvm/x86.c:5545 > [...] emulate_instruction arch/x86/include/asm/kvm_host.h:1116 > [...] complete_emulated_io arch/x86/kvm/x86.c:6870 > [...] complete_emulated_mmio+0x4e9/0x710 arch/x86/kvm/x86.c:6934 > [...] kvm_arch_vcpu_ioctl_run+0x3b7a/0x5a90 arch/x86/kvm/x86.c:6978 > [...] kvm_vcpu_ioctl+0x61e/0xdd0 > arch/x86/kvm/../../../virt/kvm/kvm_main.c:2557 > [...] vfs_ioctl fs/ioctl.c:43 > [...] do_vfs_ioctl+0x18c/0x1040 fs/ioctl.c:679 > [...] SYSC_ioctl fs/ioctl.c:694 > [...] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685 > [...] entry_SYSCALL_64_fastpath+0x1f/0xc2 > > Reported-by: Dmitry Vyukov <[email protected]> > Cc: [email protected] > Fixes: d1442d85cc30 ("KVM: x86: Handle errors when RIP is set during far > jumps") > Signed-off-by: Radim Krčmář <[email protected]> > --- > Cc: Nadav Amit <[email protected]> > Cc: Dmitry Vyukov <[email protected]> > Cc: Steve Rutherford <[email protected]> > Cc: Andrew Honig <[email protected]> > Cc: Prasad Pandit <[email protected]> > --- > arch/x86/kvm/emulate.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 7d4f9b7f06ee..d8a812b6204d 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -2137,10 +2137,7 @@ static int em_jmp_far(struct x86_emulate_ctxt *ctxt) > const struct x86_emulate_ops *ops = ctxt->ops; > u8 cpl = ctxt->ops->cpl(ctxt); > > - /* Assignment of RIP may only fail in 64-bit mode */ > - if (ctxt->mode == X86EMUL_MODE_PROT64) > - ops->get_segment(ctxt, &old_sel, &old_desc, NULL, > - VCPU_SREG_CS); > + ops->get_segment(ctxt, &old_sel, &old_desc, NULL, VCPU_SREG_CS); > > memcpy(&sel, ctxt->src.valptr + ctxt->op_bytes, 2); > > @@ -2151,12 +2148,10 @@ static int em_jmp_far(struct x86_emulate_ctxt *ctxt) > return rc; > > rc = assign_eip_far(ctxt, ctxt->src.val, &new_desc); > - if (rc != X86EMUL_CONTINUE) { > - WARN_ON(ctxt->mode != X86EMUL_MODE_PROT64); > + if (rc != X86EMUL_CONTINUE) > /* assigning eip failed; restore the old cs */ > ops->set_segment(ctxt, old_sel, &old_desc, 0, VCPU_SREG_CS); > - return rc; > - } > + > return rc; > } > > @@ -2221,9 +2216,7 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt) > struct desc_struct old_desc, new_desc; > const struct x86_emulate_ops *ops = ctxt->ops; > > - if (ctxt->mode == X86EMUL_MODE_PROT64) > - ops->get_segment(ctxt, &old_cs, &old_desc, NULL, > - VCPU_SREG_CS); > + ops->get_segment(ctxt, &old_cs, &old_desc, NULL, VCPU_SREG_CS); > > rc = emulate_pop(ctxt, &eip, ctxt->op_bytes); > if (rc != X86EMUL_CONTINUE) > @@ -2240,10 +2233,9 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt) > if (rc != X86EMUL_CONTINUE) > return rc; > rc = assign_eip_far(ctxt, eip, &new_desc); > - if (rc != X86EMUL_CONTINUE) { > - WARN_ON(ctxt->mode != X86EMUL_MODE_PROT64); > + if (rc != X86EMUL_CONTINUE) > ops->set_segment(ctxt, old_cs, &old_desc, 0, VCPU_SREG_CS); > - } > + > return rc; > } > >
Reviewed-by: Paolo Bonzini <[email protected]>
