KVM: x86: Handle errors when RIP is set during far jumps

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

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

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.

---
 arch/x86/kvm/emulate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 4b3fa70..a4ee63e 100644
@@ -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