* re: KVM: x86: Handle errors when RIP is set during far jumps
@ 2014-10-27 13:42 Dan Carpenter
2014-10-27 14:05 ` Nadav Amit
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2014-10-27 13:42 UTC (permalink / raw)
To: namit; +Cc: kvm
Hello Nadav Amit,
The patch d1442d85cc30: "KVM: x86: Handle errors when RIP is set
during far jumps" from Sep 18, 2014, leads to the following static
checker warning:
arch/x86/kvm/emulate.c:2015 em_jmp_far()
warn: add some parenthesis here?
arch/x86/kvm/emulate.c
2013 rc = assign_eip_far(ctxt, ctxt->src.val, new_desc.l);
2014 if (rc != X86EMUL_CONTINUE) {
2015 WARN_ON(!ctxt->mode != X86EMUL_MODE_PROT64);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No idea what was intended here. The negate has higher precedence than
the !=.
2016 /* assigning eip failed; restore the old cs */
2017 ops->set_segment(ctxt, old_sel, &old_desc, 0, VCPU_SREG_CS);
2018 return rc;
2019 }
There are a couple other static checker warnings as well:
arch/x86/kvm/emulate.c:579 assign_eip_far()
warn: bitwise AND condition is false here
567 static inline int assign_eip_far(struct x86_emulate_ctxt *ctxt, ulong dst,
568 int cs_l)
569 {
570 switch (ctxt->op_bytes) {
571 case 2:
572 ctxt->_eip = (u16)dst;
573 break;
574 case 4:
575 ctxt->_eip = (u32)dst;
576 break;
577 case 8:
578 if ((cs_l && is_noncanonical_address(dst)) ||
579 (!cs_l && (dst & ~(u32)-1)))
^^^^^^^^
This is a very complicated way of saying zero.
580 return emulate_gp(ctxt, 0);
581 ctxt->_eip = dst;
582 break;
583 default:
584 WARN(1, "unsupported eip assignment size\n");
585 }
586 return X86EMUL_CONTINUE;
587 }
arch/x86/kvm/emulate.c:2112 em_ret_far()
warn: add some parenthesis here?
2110 rc = assign_eip_far(ctxt, eip, new_desc.l);
2111 if (rc != X86EMUL_CONTINUE) {
2112 WARN_ON(!ctxt->mode != X86EMUL_MODE_PROT64);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2113 ops->set_segment(ctxt, old_cs, &old_desc, 0, VCPU_SREG_CS);
2114 }
2115 return rc;
2116 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: KVM: x86: Handle errors when RIP is set during far jumps
2014-10-27 13:42 KVM: x86: Handle errors when RIP is set during far jumps Dan Carpenter
@ 2014-10-27 14:05 ` Nadav Amit
2014-10-27 22:03 ` [PATCH] KVM: x86: Fix far-jump to non-canonical check Nadav Amit
0 siblings, 1 reply; 3+ messages in thread
From: Nadav Amit @ 2014-10-27 14:05 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Nadav Amit, kvm
> On Oct 27, 2014, at 15:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Nadav Amit,
>
> The patch d1442d85cc30: "KVM: x86: Handle errors when RIP is set
> during far jumps" from Sep 18, 2014, leads to the following static
> checker warning:
>
> arch/x86/kvm/emulate.c:2015 em_jmp_far()
> warn: add some parenthesis here?
Sorry for that. The fix is trivial, but let me run few tests before sending a patch, since these code-paths were not running as intended.
Nadav
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] KVM: x86: Fix far-jump to non-canonical check
2014-10-27 14:05 ` Nadav Amit
@ 2014-10-27 22:03 ` Nadav Amit
0 siblings, 0 replies; 3+ messages in thread
From: Nadav Amit @ 2014-10-27 22:03 UTC (permalink / raw)
To: pbonzini; +Cc: dan.carpenter, kvm, Nadav Amit
Commit d1442d85cc30 ("KVM: x86: Handle errors when RIP is set during far
jumps") introduced a bug that caused the fix to be incomplete. Due to
incorrect evaluation, far jump to segment with L bit cleared (i.e., 32-bit
segment) and RIP with any of the high bits set (i.e, RIP[63:32] != 0) set may
not trigger #GP. As we know, this imposes a security problem.
In addition, the condition for two warnings was incorrect.
Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
arch/x86/kvm/emulate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4b3fa70..a4ee63e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -577,7 +577,7 @@ static inline int assign_eip_far(struct x86_emulate_ctxt *ctxt, ulong dst,
break;
case 8:
if ((cs_l && is_noncanonical_address(dst)) ||
- (!cs_l && (dst & ~(u32)-1)))
+ (!cs_l && (dst >> 32) != 0))
return emulate_gp(ctxt, 0);
ctxt->_eip = dst;
break;
@@ -2016,7 +2016,7 @@ static int em_jmp_far(struct x86_emulate_ctxt *ctxt)
rc = assign_eip_far(ctxt, ctxt->src.val, new_desc.l);
if (rc != X86EMUL_CONTINUE) {
- WARN_ON(!ctxt->mode != X86EMUL_MODE_PROT64);
+ WARN_ON(ctxt->mode != X86EMUL_MODE_PROT64);
/* assigning eip failed; restore the old cs */
ops->set_segment(ctxt, old_sel, &old_desc, 0, VCPU_SREG_CS);
return rc;
@@ -2104,7 +2104,7 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
return rc;
rc = assign_eip_far(ctxt, eip, new_desc.l);
if (rc != X86EMUL_CONTINUE) {
- WARN_ON(!ctxt->mode != X86EMUL_MODE_PROT64);
+ WARN_ON(ctxt->mode != X86EMUL_MODE_PROT64);
ops->set_segment(ctxt, old_cs, &old_desc, 0, VCPU_SREG_CS);
}
return rc;
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-27 22:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-27 13:42 KVM: x86: Handle errors when RIP is set during far jumps Dan Carpenter
2014-10-27 14:05 ` Nadav Amit
2014-10-27 22:03 ` [PATCH] KVM: x86: Fix far-jump to non-canonical check Nadav Amit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.