Re: [PATCH] KVM: x86: restore IP after all far jump failures

[Jim Mattson]

Should the subject read: "KVM: x86: restore CS after all far jump failures"?

On Tue, Nov 22, 2016 at 11:21 AM, Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> 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 <dvyukov@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: d1442d85cc30 ("KVM: x86: Handle errors when RIP is set during far jumps")
> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> ---
> Cc: Nadav Amit <nadav.amit@xxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Steve Rutherford <srutherford@xxxxxxxxxx>
> Cc: Andrew Honig <ahonig@xxxxxxxxxx>
> Cc: Prasad Pandit <ppandit@xxxxxxxxxx>
> ---
>  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;
>  }
>
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html