[RFC PATCH v2 0/3] Add segment limit checks to emulator

[RFC PATCH v2 0/3] Add segment limit checks to emulator

[Mohammed Gamal]

After some conversation with Avi concerning why unreal mode has been seen to work
with KVM on Intel. It clears out the scenario is caused as follows:

- guest enters big real mode
- kvm squashes limit to 64k-1
- guest executes instructions with offset > 64k
- cpu issues #GP due to limit violation
- kvm handle_rmode_exception() -> emulator
- emulator ignores limit, emulates instruction

With these applied I am getting vmentry failures with SeaBIOS and
gPXE. I could still get SeaBIOS to work with emulate_invalid_guest_state=1.
So it's needless to say that these patches are not meant for merging!

--------

Changes from v1:
- Added seg_override_limit() and emulate_ss() helpers in emulator
- Corrected limit check return values for long mode
- Limit Checking is now consistent with the Intel documentation

--------

Mohammed Gamal (3):
  Add helper methods to get segment limits
  x86 emulator: Add cs_base() helper
  x86 emulator: Add segment limit checks and helper functions

 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/include/asm/kvm_host.h    |    1 +
 arch/x86/kvm/emulate.c             |  143 ++++++++++++++++++++++++++++++------
 arch/x86/kvm/svm.c                 |    8 ++
 arch/x86/kvm/vmx.c                 |    8 ++
 arch/x86/kvm/x86.c                 |   12 +++
 6 files changed, 150 insertions(+), 23 deletions(-)

[RFC PATCH v2 1/3] Add helper methods to get segment limits

[Mohammed Gamal]

This adds helper methods to get segment limits for kvm_x86_ops and x86_emulate_ops. Hooks are added for SVM and VMX

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/include/asm/kvm_host.h    |    1 +
 arch/x86/kvm/svm.c                 |    8 ++++++++
 arch/x86/kvm/vmx.c                 |    8 ++++++++
 arch/x86/kvm/x86.c                 |   12 ++++++++++++
 5 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 51cfd73..ce90048 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -138,6 +138,7 @@ struct x86_emulate_ops {
 	u16 (*get_segment_selector)(int seg, struct kvm_vcpu *vcpu);
 	void (*set_segment_selector)(u16 sel, int seg, struct kvm_vcpu *vcpu);
 	unsigned long (*get_cached_segment_base)(int seg, struct kvm_vcpu *vcpu);
+	u32 (*get_cached_segment_limit)(int seg, struct kvm_vcpu *vcpu);
 	void (*get_gdt)(struct desc_ptr *dt, struct kvm_vcpu *vcpu);
 	ulong (*get_cr)(int cr, struct kvm_vcpu *vcpu);
 	int (*set_cr)(int cr, ulong val, struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 502e53f..e32efc4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -468,6 +468,7 @@ struct kvm_x86_ops {
 	int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata);
 	int (*set_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
 	u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
+	u32 (*get_segment_limit)(struct kvm_vcpu *vcpu, int seg);
 	void (*get_segment)(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg);
 	int (*get_cpl)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 56c9b6b..504761d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1038,6 +1038,13 @@ static u64 svm_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 	return s->base;
 }
 
+static u32 svm_get_segment_limit(struct kvm_vcpu *vcpu, int seg)
+{
+	struct vmcb_seg *s = svm_seg(vcpu, seg);
+
+	return s->limit;
+}
+
 static void svm_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
@@ -3461,6 +3468,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.get_msr = svm_get_msr,
 	.set_msr = svm_set_msr,
 	.get_segment_base = svm_get_segment_base,
+	.get_segment_limit = svm_get_segment_limit,
 	.get_segment = svm_get_segment,
 	.set_segment = svm_set_segment,
 	.get_cpl = svm_get_cpl,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2fdcc98..4e2626b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2001,6 +2001,13 @@ static u64 vmx_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 	return vmcs_readl(sf->base);
 }
 
+static u32 vmx_get_segment_limit(struct kvm_vcpu *vcpu, int seg)
+{
+	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
+
+	return vmcs_read32(sf->limit);
+}
+
 static void vmx_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
@@ -4304,6 +4311,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.get_msr = vmx_get_msr,
 	.set_msr = vmx_set_msr,
 	.get_segment_base = vmx_get_segment_base,
+	.get_segment_limit = vmx_get_segment_limit,
 	.get_segment = vmx_get_segment,
 	.set_segment = vmx_set_segment,
 	.get_cpl = vmx_get_cpl,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6ed3176..d5a231b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3674,6 +3674,11 @@ static unsigned long get_segment_base(struct kvm_vcpu *vcpu, int seg)
 	return kvm_x86_ops->get_segment_base(vcpu, seg);
 }
 
+static u32 get_segment_limit (struct kvm_vcpu *vcpu, int seg)
+{
+	return kvm_x86_ops->get_segment_limit(vcpu, seg);
+}
+
 int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)
 {
 	kvm_mmu_invlpg(vcpu, address);
@@ -3790,6 +3795,12 @@ static unsigned long emulator_get_cached_segment_base(int seg,
 	return get_segment_base(vcpu, seg);
 }
 
+static u32 emulate_get_cached_segment_limit(int seg,
+					    struct kvm_vcpu *vcpu)
+{
+	return get_segment_limit(vcpu, seg);
+}
+
 static bool emulator_get_cached_descriptor(struct desc_struct *desc, int seg,
 					   struct kvm_vcpu *vcpu)
 {
@@ -3876,6 +3887,7 @@ static struct x86_emulate_ops emulate_ops = {
 	.get_segment_selector = emulator_get_segment_selector,
 	.set_segment_selector = emulator_set_segment_selector,
 	.get_cached_segment_base = emulator_get_cached_segment_base,
+	.get_cached_segment_limit = emulate_get_cached_segment_limit,
 	.get_gdt             = emulator_get_gdt,
 	.get_cr              = emulator_get_cr,
 	.set_cr              = emulator_set_cr,
-- 
1.7.0.4

[RFC PATCH v2 2/3] x86 emulator: Add cs_base() helper

[Mohammed Gamal]

Adds a cs_base() helper for consistency with other emulator functions

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/emulate.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 255473f..6bb7c68 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -643,6 +643,12 @@ static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
 	return seg_base(ctxt, ops, c->seg_override);
 }
 
+static unsigned long cs_base(struct x86_emulate_ctxt *ctxt,
+			     struct x86_emulate_ops *ops)
+{
+	return seg_base(ctxt, ops, VCPU_SREG_CS);
+}
+
 static unsigned long es_base(struct x86_emulate_ctxt *ctxt,
 			     struct x86_emulate_ops *ops)
 {
@@ -976,7 +982,7 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 
 	c->eip = ctxt->eip;
 	c->fetch.start = c->fetch.end = c->eip;
-	ctxt->cs_base = seg_base(ctxt, ops, VCPU_SREG_CS);
+	ctxt->cs_base = cs_base(ctxt, ops);
 
 	switch (mode) {
 	case X86EMUL_MODE_REAL:
-- 
1.7.0.4

[RFC PATCH v2 3/3] x86 emulator: Add segment limit checks and helper functions

[Mohammed Gamal]

This patch adds segment limit checks to the x86 emulator, in addition to some
helper functions and changes to the return values of emulate_push to accomodate
the new checks.

----

Changes from v1:
- Added seg_override_limit() and emulate_ss() helpers
- Corrected limit check return values for long mode
- Limit Checking is now consistent with the Intel documentation

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/emulate.c |  135 ++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 113 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 6bb7c68..f415d4b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -633,6 +633,15 @@ static unsigned long seg_base(struct x86_emulate_ctxt *ctxt,
 	return ops->get_cached_segment_base(seg, ctxt->vcpu);
 }
 
+static u32 seg_limit(struct x86_emulate_ctxt *ctxt,
+		     struct x86_emulate_ops *ops, int seg)
+{
+	if (ctxt->mode == X86EMUL_MODE_PROT64)
+		return -1;
+
+	return ops->get_cached_segment_limit(seg, ctxt->vcpu);
+}
+
 static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
 				       struct x86_emulate_ops *ops,
 				       struct decode_cache *c)
@@ -643,6 +652,16 @@ static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
 	return seg_base(ctxt, ops, c->seg_override);
 }
 
+static u32 seg_override_limit(struct x86_emulate_ctxt *ctxt,
+				       struct x86_emulate_ops *ops,
+				       struct decode_cache *c)
+{
+	if (!c->has_seg_override)
+		return -1;
+	
+	return seg_limit(ctxt, ops, c->seg_override);
+}
+
 static unsigned long cs_base(struct x86_emulate_ctxt *ctxt,
 			     struct x86_emulate_ops *ops)
 {
@@ -661,6 +680,24 @@ static unsigned long ss_base(struct x86_emulate_ctxt *ctxt,
 	return seg_base(ctxt, ops, VCPU_SREG_SS);
 }
 
+static u32 cs_limit(struct x86_emulate_ctxt *ctxt,
+		    struct x86_emulate_ops *ops)
+{
+	return seg_limit(ctxt, ops, VCPU_SREG_CS);
+}
+
+static u32 es_limit(struct x86_emulate_ctxt *ctxt,
+		    struct x86_emulate_ops *ops)
+{
+	return seg_limit(ctxt, ops, VCPU_SREG_ES);
+}
+
+static u32 ss_limit(struct x86_emulate_ctxt *ctxt,
+		    struct x86_emulate_ops *ops)
+{
+	return seg_limit(ctxt, ops, VCPU_SREG_SS);
+}
+
 static void emulate_exception(struct x86_emulate_ctxt *ctxt, int vec,
 				      u32 error, bool valid)
 {
@@ -675,6 +712,11 @@ static void emulate_gp(struct x86_emulate_ctxt *ctxt, int err)
 	emulate_exception(ctxt, GP_VECTOR, err, true);
 }
 
+static void emulate_ss(struct x86_emulate_ctxt *ctxt, int err)
+{
+	emulate_exception(ctxt, SS_VECTOR, err, true);
+}
+
 static void emulate_pf(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 		       int err)
 {
@@ -719,6 +761,12 @@ static int do_insn_fetch(struct x86_emulate_ctxt *ctxt,
 {
 	int rc;
 
+	/* eip is already relative to CS, so we just check it against the limit */
+	if (eip > cs_limit(ctxt, ops) - size - 1) {
+		emulate_gp(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
 	/* x86 instructions are limited to 15 bytes. */
 	if (eip + size - ctxt->eip > 15)
 		return X86EMUL_UNHANDLEABLE;
@@ -1222,6 +1270,11 @@ done_prefixes:
 		c->src.ptr = (unsigned long *)
 			register_address(c,  seg_override_base(ctxt, ops, c),
 					 c->regs[VCPU_REGS_RSI]);
+		if ((unsigned long)c->src.ptr - seg_override_base(ctxt, ops, c) > 
+			seg_override_limit(ctxt, ops, c) - c->src.bytes - 1) {
+			emulate_gp(ctxt, 0);
+			return X86EMUL_PROPAGATE_FAULT;
+		}
 		c->src.val = 0;
 		break;
 	case SrcImmFAddr:
@@ -1318,6 +1371,12 @@ done_prefixes:
 		c->dst.ptr = (unsigned long *)
 			register_address(c, es_base(ctxt, ops),
 					 c->regs[VCPU_REGS_RDI]);
+
+		if ((unsigned long)c->dst.ptr - es_base(ctxt, ops) >
+			es_limit(ctxt, ops) - c->dst.bytes - 1) {
+			emulate_gp(ctxt, 0);
+			return X86EMUL_PROPAGATE_FAULT;
+		}
 		c->dst.val = 0;
 		break;
 	}
@@ -1637,7 +1696,7 @@ static inline int writeback(struct x86_emulate_ctxt *ctxt,
 	return X86EMUL_CONTINUE;
 }
 
-static inline void emulate_push(struct x86_emulate_ctxt *ctxt,
+static inline int emulate_push(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -1648,6 +1707,13 @@ static inline void emulate_push(struct x86_emulate_ctxt *ctxt,
 	register_address_increment(c, &c->regs[VCPU_REGS_RSP], -c->op_bytes);
 	c->dst.ptr = (void *) register_address(c, ss_base(ctxt, ops),
 					       c->regs[VCPU_REGS_RSP]);
+	if ((unsigned long) c->dst.ptr - ss_base(ctxt, ops) >
+		ss_limit(ctxt, ops) - c->dst.bytes - 1) {
+		emulate_ss(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	return X86EMUL_CONTINUE;
 }
 
 static int emulate_pop(struct x86_emulate_ctxt *ctxt,
@@ -1655,11 +1721,15 @@ static int emulate_pop(struct x86_emulate_ctxt *ctxt,
 		       void *dest, int len)
 {
 	struct decode_cache *c = &ctxt->decode;
+	unsigned long reg_addr = register_address(c, ss_base(ctxt, ops), c->regs[VCPU_REGS_RSP]);
 	int rc;
 
-	rc = read_emulated(ctxt, ops, register_address(c, ss_base(ctxt, ops),
-						       c->regs[VCPU_REGS_RSP]),
-			   dest, len);
+	if (reg_addr - ss_base(ctxt, ops) > ss_limit(ctxt, ops) - c->src.bytes - 1) {
+		emulate_ss(ctxt, 0);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	rc = read_emulated(ctxt, ops, reg_addr, dest, len);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
@@ -1710,14 +1780,14 @@ static int emulate_popf(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
-static void emulate_push_sreg(struct x86_emulate_ctxt *ctxt,
+static int emulate_push_sreg(struct x86_emulate_ctxt *ctxt,
 			      struct x86_emulate_ops *ops, int seg)
 {
 	struct decode_cache *c = &ctxt->decode;
 
 	c->src.val = ops->get_segment_selector(seg, ctxt->vcpu);
 
-	emulate_push(ctxt, ops);
+	return emulate_push(ctxt, ops);
 }
 
 static int emulate_pop_sreg(struct x86_emulate_ctxt *ctxt,
@@ -1747,7 +1817,10 @@ static int emulate_pusha(struct x86_emulate_ctxt *ctxt,
 		(reg == VCPU_REGS_RSP) ?
 		(c->src.val = old_esp) : (c->src.val = c->regs[reg]);
 
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
 
 		rc = writeback(ctxt, ops);
 		if (rc != X86EMUL_CONTINUE)
@@ -1859,15 +1932,13 @@ static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
 		old_eip = c->eip;
 		c->eip = c->src.val;
 		c->src.val = old_eip;
-		emulate_push(ctxt, ops);
-		break;
+		return emulate_push(ctxt, ops);
 	}
 	case 4: /* jmp abs */
 		c->eip = c->src.val;
 		break;
 	case 6:	/* push */
-		emulate_push(ctxt, ops);
-		break;
+		return emulate_push(ctxt, ops);
 	}
 	return X86EMUL_CONTINUE;
 }
@@ -2523,7 +2594,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 		c->op_bytes = c->ad_bytes = (next_tss_desc.type & 8) ? 4 : 2;
 		c->lock_prefix = 0;
 		c->src.val = (unsigned long) error_code;
-		emulate_push(ctxt, ops);
+		ret = emulate_push(ctxt, ops);
 	}
 
 	return ret;
@@ -2656,7 +2727,9 @@ special_insn:
 		emulate_2op_SrcV("add", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x06:		/* push es */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x07:		/* pop es */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_ES);
@@ -2668,14 +2741,18 @@ special_insn:
 		emulate_2op_SrcV("or", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x0e:		/* push cs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x10 ... 0x15:
 	      adc:		/* adc */
 		emulate_2op_SrcV("adc", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x16:		/* push ss */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x17:		/* pop ss */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_SS);
@@ -2687,7 +2764,9 @@ special_insn:
 		emulate_2op_SrcV("sbb", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x1e:		/* push ds */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x1f:		/* pop ds */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_DS);
@@ -2717,7 +2796,9 @@ special_insn:
 		emulate_1op("dec", c->dst, ctxt->eflags);
 		break;
 	case 0x50 ... 0x57:  /* push reg */
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x58 ... 0x5f: /* pop reg */
 	pop_instruction:
@@ -2742,7 +2823,9 @@ special_insn:
 		break;
 	case 0x68: /* push imm */
 	case 0x6a: /* push imm8 */
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x6c:		/* insb */
 	case 0x6d:		/* insw/insd */
@@ -2870,7 +2953,9 @@ special_insn:
 		goto xchg;
 	case 0x9c: /* pushf */
 		c->src.val =  (unsigned long) ctxt->eflags;
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x9d: /* popf */
 		c->dst.type = OP_REG;
@@ -2934,7 +3019,9 @@ special_insn:
 		long int rel = c->src.val;
 		c->src.val = (unsigned long) c->eip;
 		jmp_rel(c, rel);
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	}
 	case 0xe9: /* jmp rel */
@@ -3261,7 +3348,9 @@ twobyte_insn:
 		c->dst.type = OP_NONE;
 		break;
 	case 0xa0:	  /* push fs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_FS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_FS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0xa1:	 /* pop fs */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_FS);
@@ -3280,7 +3369,9 @@ twobyte_insn:
 		emulate_2op_cl("shld", c->src2, c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xa8:	/* push gs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_GS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_GS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0xa9:	/* pop gs */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_GS);
-- 
1.7.0.4

Re: [RFC PATCH v2 3/3] x86 emulator: Add segment limit checks and helper functions

[Avi Kivity]

On 07/08/2010 05:10 PM, Mohammed Gamal wrote:
> This patch adds segment limit checks to the x86 emulator, in addition to some
> helper functions and changes to the return values of emulate_push to accomodate
> the new checks.
>
>
>
> +static u32 seg_limit(struct x86_emulate_ctxt *ctxt,
> +		     struct x86_emulate_ops *ops, int seg)
> +{
> +	if (ctxt->mode == X86EMUL_MODE_PROT64)
> +		return -1;
>    

That doesn't work well for 64 bit.  It returns 4G-1, you want 2^64-1.  
The return type needs to be u64 and the value -1ULL.


> +
> +	return ops->get_cached_segment_limit(seg, ctxt->vcpu);
> +}
> +
>   static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
>   				       struct x86_emulate_ops *ops,
>   				       struct decode_cache *c)
>
>   static void emulate_pf(struct x86_emulate_ctxt *ctxt, unsigned long addr,
>   		       int err)
>   {
> @@ -719,6 +761,12 @@ static int do_insn_fetch(struct x86_emulate_ctxt *ctxt,
>   {
>   	int rc;
>
> +	/* eip is already relative to CS, so we just check it against the limit */
> +	if (eip>  cs_limit(ctxt, ops) - size - 1) {
>    

What if

    eip = 1
    limit = 3
    size = 8?

> +		emulate_gp(ctxt, 0);
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +
>   	/* x86 instructions are limited to 15 bytes. */
>   	if (eip + size - ctxt->eip>  15)
>   		return X86EMUL_UNHANDLEABLE;
> @@ -1222,6 +1270,11 @@ done_prefixes:
>   		c->src.ptr = (unsigned long *)
>   			register_address(c,  seg_override_base(ctxt, ops, c),
>   					 c->regs[VCPU_REGS_RSI]);
> +		if ((unsigned long)c->src.ptr - seg_override_base(ctxt, ops, c)>
> +			seg_override_limit(ctxt, ops, c) - c->src.bytes - 1) {
> +			emulate_gp(ctxt, 0);
> +			return X86EMUL_PROPAGATE_FAULT;
> +		}
>    

Similar issue.

This code is repeated two often. Best to have a helper for reading and 
writing that accepts the segment ID, which does the limit check (don't 
forget expand-down segments), then the read/write/fetch or #GP/#SS.

That helper can also add the base, so it reduces code elsewhere.  I 
suggest a first patch that introduces the read/write/fetch helpers, and 
a second patch that adds the limit checks.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.