Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934106AbcKVToz (ORCPT ); Tue, 22 Nov 2016 14:44:55 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:33802 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934007AbcKVTox (ORCPT ); Tue, 22 Nov 2016 14:44:53 -0500 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.1 \(3251\)) Subject: Re: [PATCH] KVM: x86: restore IP after all far jump failures From: Nadav Amit In-Reply-To: Date: Tue, 22 Nov 2016 11:44:51 -0800 Cc: =?utf-8?B?UmFkaW0gS3LEjW3DocWZ?= , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, stable@vger.kernel.org, Dmitry Vyukov , Steve Rutherford , Andrew Honig , Prasad Pandit Message-Id: <4E272B11-A8B9-4E55-9E30-EB8921890CED@gmail.com> References: <20161122192104.19836-1-rkrcmar@redhat.com> To: Paolo Bonzini X-Mailer: Apple Mail (2.3251) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAMJj9PW006865 Content-Length: 4866 Lines: 125 I admit my wrongdoings, but I still think the fix should have been to remove the entire recovery logic and just return X86EMUL_UNHANDLEABLE if something goes wrong (exception). This will kill the misbehaving process but keep the VM running. Otherwise, a malicious VM process, which can somehow control descriptors (LDT?) may modify the descriptor during the emulation and get the system to inconsistent state and prevent the VM-entry. No? Nadav > On Nov 22, 2016, at 11:34 AM, Paolo Bonzini wrote: > > > > 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 >> Cc: stable@vger.kernel.org >> Fixes: d1442d85cc30 ("KVM: x86: Handle errors when RIP is set during far jumps") >> Signed-off-by: Radim Krčmář >> --- >> Cc: Nadav Amit >> Cc: Dmitry Vyukov >> Cc: Steve Rutherford >> Cc: Andrew Honig >> Cc: Prasad Pandit >> --- >> 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