* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox