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.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to