x86emul: limit-check branch targets

All branches need to #GP when their target violates the segment limit
(in 16- and 32-bit modes) or is non-canonical (in 64-bit mode). For
near branches facilitate this via a zero-byte instruction fetch from
the target address (resulting in address translation and validation
without an actual read from memory), while far branches get dealt with
by breaking up the segment register loading into a read-and-validate
part and a write one. The latter at once allows correcting some
ordering issues in how the individual emulation steps get carried out:
Before updating machine state, all exceptions unrelated to that state
updating should have got raised (i.e. the only ones possibly resulting
in partly updated state are faulting memory writes [pushes]).

Note that while not immediately needed here, write and distinct read
emulation routines get updated to deal with zero byte accesses too, for
overall consistency.

Reported-by: 刘令 <liuling-it@360.cn>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
master commit: 81d3a0b26c1672c60b2a54dd8780e6f6472d2328
master date: 2016-02-26 12:14:39 +0100

diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c
index ef9bfe99a116245a894f2596cad0068d9ee4bf50..084012bf34c7e3e0f0e10187f55bace3cba5138b 100644 (file)
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -8,6 +8,8 @@
 
 typedef bool bool_t;
 
+#define is_canonical_address(x) (((int64_t)(x) >> 47) == ((int64_t)(x) >> 63))
+
 #define BUG() abort()
 #define ASSERT assert
 

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 14c184740be37cdec75169484bfdc095ac252686..5cea077033862688c320e267023db26896f83e10 100644 (file)
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -472,7 +472,7 @@ static int __hvmemul_read(
 
     rc = hvmemul_virtual_to_linear(
         seg, offset, bytes, &reps, access_type, hvmemul_ctxt, &addr);
-    if ( rc != X86EMUL_OKAY )
+    if ( rc != X86EMUL_OKAY || !bytes )
         return rc;
     off = addr & (PAGE_SIZE - 1);
     /*
@@ -582,13 +582,17 @@ static int hvmemul_insn_fetch(
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
     unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip;
 
-    /* Fall back if requested bytes are not in the prefetch cache. */
-    if ( unlikely((insn_off + bytes) > hvmemul_ctxt->insn_buf_bytes) )
+    /*
+     * Fall back if requested bytes are not in the prefetch cache.
+     * But always perform the (fake) read when bytes == 0.
+     */
+    if ( !bytes ||
+         unlikely((insn_off + bytes) > hvmemul_ctxt->insn_buf_bytes) )
     {
         int rc = __hvmemul_read(seg, offset, p_data, bytes,
                                 hvm_access_insn_fetch, hvmemul_ctxt);
 
-        if ( rc == X86EMUL_OKAY )
+        if ( rc == X86EMUL_OKAY && bytes )
         {
             ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf));
             memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes);
@@ -622,7 +626,7 @@ static int hvmemul_write(
 
     rc = hvmemul_virtual_to_linear(
         seg, offset, bytes, &reps, hvm_access_write, hvmemul_ctxt, &addr);
-    if ( rc != X86EMUL_OKAY )
+    if ( rc != X86EMUL_OKAY || !bytes )
         return rc;
     off = addr & (PAGE_SIZE - 1);
     /* See the respective comment in __hvmemul_read(). */

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ac3fa3123c931cc5b387f1132dea04455b0ceca8..91518486cb401a6aba241dfd8288d397c064f7b0 100644 (file)
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3383,7 +3383,7 @@ int hvm_virtual_to_linear_addr(
          * Certain of them are not done in native real mode anyway.
          */
         addr = (uint32_t)(addr + reg->base);
-        last_byte = (uint32_t)addr + bytes - 1;
+        last_byte = (uint32_t)addr + bytes - !!bytes;
         if ( last_byte < addr )
             return 0;
     }
@@ -3407,7 +3407,7 @@ int hvm_virtual_to_linear_addr(
             break;
         }
 
-        last_byte = (uint32_t)offset + bytes - 1;
+        last_byte = (uint32_t)offset + bytes - !!bytes;
 
         /* Is this a grows-down data segment? Special limit check if so. */
         if ( (reg->attr.fields.type & 0xc) == 0x4 )
@@ -3438,7 +3438,7 @@ int hvm_virtual_to_linear_addr(
         if ( (seg == x86_seg_fs) || (seg == x86_seg_gs) )
             addr += reg->base;
 
-        last_byte = addr + bytes - 1;
+        last_byte = addr + bytes - !!bytes;
         if ( !is_canonical_address(addr) || last_byte < addr ||
              !is_canonical_address(last_byte) )
             return 0;

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 3b522708fb397b1b138976ebf7297d3df4729838..e534bd1e1e41c6e87411e3a3d1ab715ef94955af 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4942,10 +4942,11 @@ static int ptwr_emulated_read(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
-    unsigned int rc;
+    unsigned int rc = bytes;
     unsigned long addr = offset;
 
-    if ( (rc = copy_from_user(p_data, (void *)addr, bytes)) != 0 )
+    if ( !__addr_ok(addr) ||
+         (rc = __copy_from_user(p_data, (void *)addr, bytes)) )
     {
         propagate_page_fault(addr + bytes - rc, 0); /* read fault */
         return X86EMUL_EXCEPTION;
@@ -5094,7 +5095,7 @@ static int ptwr_emulated_write(
 {
     paddr_t val = 0;
 
-    if ( (bytes > sizeof(paddr_t)) || (bytes & (bytes -1)) )
+    if ( (bytes > sizeof(paddr_t)) || (bytes & (bytes - 1)) || !bytes )
     {
         MEM_LOG("ptwr_emulate: bad write size (addr=%lx, bytes=%u)",
                 offset, bytes);
@@ -5225,7 +5226,8 @@ static int mmio_ro_emulated_write(
         container_of(ctxt, struct mmio_ro_emulate_ctxt, ctxt);
 
     /* Only allow naturally-aligned stores at the original %cr2 address. */
-    if ( ((bytes | offset) & (bytes - 1)) || offset != mmio_ro_ctxt->cr2 )
+    if ( ((bytes | offset) & (bytes - 1)) || !bytes ||
+         offset != mmio_ro_ctxt->cr2 )
     {
         MEM_LOG("mmio_ro_emulate: bad access (cr2=%lx, addr=%lx, bytes=%u)",
                 mmio_ro_ctxt->cr2, offset, bytes);

diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 18026fe01384cb2c8979b2a227c10307d27ef25c..f3b64f1186c065e891093ea4b97115c02d72a911 100644 (file)
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -155,7 +155,7 @@ hvm_read(enum x86_segment seg,
 
     rc = hvm_translate_linear_addr(
         seg, offset, bytes, access_type, sh_ctxt, &addr);
-    if ( rc )
+    if ( rc || !bytes )
         return rc;
 
     if ( access_type == hvm_access_insn_fetch )
@@ -239,7 +239,7 @@ hvm_emulate_write(enum x86_segment seg,
 
     rc = hvm_translate_linear_addr(
         seg, offset, bytes, hvm_access_write, sh_ctxt, &addr);
-    if ( rc )
+    if ( rc || !bytes )
         return rc;
 
     return v->arch.paging.mode->shadow.x86_emulate_write(

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 12eb2ddd37b2a14dc1cdf3151f09f279c36f4dc0..5eac889a22bd2842f4c2094a858508d71c2a1817 100644 (file)
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -652,14 +652,26 @@ do {                                                                    \
 
 #define jmp_rel(rel)                                                    \
 do {                                                                    \
-    int _rel = (int)(rel);                                              \
-    _regs.eip += _rel;                                                  \
+    unsigned long ip = _regs.eip + (int)(rel);                          \
     if ( op_bytes == 2 )                                                \
-        _regs.eip = (uint16_t)_regs.eip;                                \
+        ip = (uint16_t)ip;                                              \
     else if ( !mode_64bit() )                                           \
-        _regs.eip = (uint32_t)_regs.eip;                                \
+        ip = (uint32_t)ip;                                              \
+    rc = ops->insn_fetch(x86_seg_cs, ip, NULL, 0, ctxt);                \
+    if ( rc ) goto done;                                                \
+    _regs.eip = ip;                                                     \
 } while (0)
 
+#define validate_far_branch(cs, ip)                                     \
+    generate_exception_if(in_longmode(ctxt, ops) && (cs)->attr.fields.l \
+                          ? !is_canonical_address(ip)                   \
+                          : (ip) > (cs)->limit, EXC_GP, 0)
+
+#define commit_far_branch(cs, ip) ({                                    \
+    validate_far_branch(cs, ip);                                        \
+    ops->write_segment(x86_seg_cs, cs, ctxt);                           \
+})
+
 struct fpu_insn_ctxt {
     uint8_t insn_bytes;
     uint8_t exn_raised;
@@ -1104,29 +1116,30 @@ static int
 realmode_load_seg(
     enum x86_segment seg,
     uint16_t sel,
+    struct segment_register *sreg,
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops *ops)
 {
-    struct segment_register reg;
-    int rc;
-
-    if ( (rc = ops->read_segment(seg, &reg, ctxt)) != 0 )
-        return rc;
+    int rc = ops->read_segment(seg, sreg, ctxt);
 
-    reg.sel  = sel;
-    reg.base = (uint32_t)sel << 4;
+    if ( !rc )
+    {
+        sreg->sel  = sel;
+        sreg->base = (uint32_t)sel << 4;
+    }
 
-    return ops->write_segment(seg, &reg, ctxt);
+    return rc;
 }
 
 static int
 protmode_load_seg(
     enum x86_segment seg,
     uint16_t sel, bool_t is_ret,
+    struct segment_register *sreg,
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops *ops)
 {
-    struct segment_register desctab, ss, segr;
+    struct segment_register desctab, ss;
     struct { uint32_t a, b; } desc;
     uint8_t dpl, rpl, cpl;
     uint32_t new_desc_b, a_flag = 0x100;
@@ -1137,8 +1150,8 @@ protmode_load_seg(
     {
         if ( (seg == x86_seg_cs) || (seg == x86_seg_ss) )
             goto raise_exn;
-        memset(&segr, 0, sizeof(segr));
-        return ops->write_segment(seg, &segr, ctxt);
+        memset(sreg, 0, sizeof(*sreg));
+        return X86EMUL_OKAY;
     }
 
     /* System segment descriptors must reside in the GDT. */
@@ -1247,16 +1260,16 @@ protmode_load_seg(
     desc.b |= a_flag;
 
  skip_accessed_flag:
-    segr.base = (((desc.b <<  0) & 0xff000000u) |
-                 ((desc.b << 16) & 0x00ff0000u) |
-                 ((desc.a >> 16) & 0x0000ffffu));
-    segr.attr.bytes = (((desc.b >>  8) & 0x00ffu) |
-                       ((desc.b >> 12) & 0x0f00u));
-    segr.limit = (desc.b & 0x000f0000u) | (desc.a & 0x0000ffffu);
-    if ( segr.attr.fields.g )
-        segr.limit = (segr.limit << 12) | 0xfffu;
-    segr.sel = sel;
-    return ops->write_segment(seg, &segr, ctxt);
+    sreg->base = (((desc.b <<  0) & 0xff000000u) |
+                  ((desc.b << 16) & 0x00ff0000u) |
+                  ((desc.a >> 16) & 0x0000ffffu));
+    sreg->attr.bytes = (((desc.b >>  8) & 0x00ffu) |
+                        ((desc.b >> 12) & 0x0f00u));
+    sreg->limit = (desc.b & 0x000f0000u) | (desc.a & 0x0000ffffu);
+    if ( sreg->attr.fields.g )
+        sreg->limit = (sreg->limit << 12) | 0xfffu;
+    sreg->sel = sel;
+    return X86EMUL_OKAY;
 
  raise_exn:
     if ( ops->inject_hw_exception == NULL )
@@ -1270,17 +1283,29 @@ static int
 load_seg(
     enum x86_segment seg,
     uint16_t sel, bool_t is_ret,
+    struct segment_register *sreg,
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops *ops)
 {
+    struct segment_register reg;
+    int rc;
+
     if ( (ops->read_segment == NULL) ||
          (ops->write_segment == NULL) )
         return X86EMUL_UNHANDLEABLE;
 
+    if ( !sreg )
+        sreg = &reg;
+
     if ( in_protmode(ctxt, ops) )
-        return protmode_load_seg(seg, sel, is_ret, ctxt, ops);
+        rc = protmode_load_seg(seg, sel, is_ret, sreg, ctxt, ops);
+    else
+        rc = realmode_load_seg(seg, sel, sreg, ctxt, ops);
+
+    if ( !rc && sreg == &reg )
+        rc = ops->write_segment(seg, sreg, ctxt);
 
-    return realmode_load_seg(seg, sel, ctxt, ops);
+    return rc;
 }
 
 void *
@@ -1964,6 +1989,8 @@ x86_emulate(
 
     switch ( b )
     {
+        struct segment_register cs;
+
     case 0x00 ... 0x05: add: /* add */
         emulate_2op_SrcV("add", src, dst, _regs.eflags);
         break;
@@ -2025,7 +2052,7 @@ x86_emulate(
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &dst.val, op_bytes, ctxt, ops)) != 0 )
             goto done;
-        if ( (rc = load_seg(src.val, dst.val, 0, ctxt, ops)) != 0 )
+        if ( (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 )
             return rc;
         break;
 
@@ -2379,7 +2406,7 @@ x86_emulate(
         enum x86_segment seg = decode_segment(modrm_reg);
         generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
         generate_exception_if(seg == x86_seg_cs, EXC_UD, -1);
-        if ( (rc = load_seg(seg, src.val, 0, ctxt, ops)) != 0 )
+        if ( (rc = load_seg(seg, src.val, 0, NULL, ctxt, ops)) != 0 )
             goto done;
         if ( seg == x86_seg_ss )
             ctxt->retire.flags.mov_ss = 1;
@@ -2454,14 +2481,15 @@ x86_emulate(
         sel = insn_fetch_type(uint16_t);
 
         if ( (rc = ops->read_segment(x86_seg_cs, &reg, ctxt)) ||
-             (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
+             (rc = load_seg(x86_seg_cs, sel, 0, &cs, ctxt, ops)) ||
+             (validate_far_branch(&cs, eip),
+              rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
                               &reg.sel, op_bytes, ctxt)) ||
              (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
-                              &_regs.eip, op_bytes, ctxt)) )
+                              &_regs.eip, op_bytes, ctxt)) ||
+             (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) )
             goto done;
 
-        if ( (rc = load_seg(x86_seg_cs, sel, 0, ctxt, ops)) != 0 )
-            goto done;
         _regs.eip = eip;
         break;
     }
@@ -2669,7 +2697,8 @@ x86_emulate(
         int offset = (b == 0xc2) ? insn_fetch_type(uint16_t) : 0;
         op_bytes = ((op_bytes == 4) && mode_64bit()) ? 8 : op_bytes;
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + offset),
-                              &dst.val, op_bytes, ctxt, ops)) != 0 )
+                              &dst.val, op_bytes, ctxt, ops)) != 0 ||
+             (rc = ops->insn_fetch(x86_seg_cs, dst.val, NULL, 0, ctxt)) )
             goto done;
         _regs.eip = dst.val;
         break;
@@ -2684,7 +2713,7 @@ x86_emulate(
         if ( (rc = read_ulong(src.mem.seg, src.mem.off + src.bytes,
                               &sel, 2, ctxt, ops)) != 0 )
             goto done;
-        if ( (rc = load_seg(dst.val, sel, 0, ctxt, ops)) != 0 )
+        if ( (rc = load_seg(dst.val, sel, 0, NULL, ctxt, ops)) != 0 )
             goto done;
         dst.val = src.val;
         break;
@@ -2758,7 +2787,8 @@ x86_emulate(
                               &dst.val, op_bytes, ctxt, ops)) ||
              (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + offset),
                               &src.val, op_bytes, ctxt, ops)) ||
-             (rc = load_seg(x86_seg_cs, src.val, 1, ctxt, ops)) )
+             (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
+             (rc = commit_far_branch(&cs, dst.val)) )
             goto done;
         _regs.eip = dst.val;
         break;
@@ -2787,7 +2817,7 @@ x86_emulate(
         goto swint;
 
     case 0xcf: /* iret */ {
-        unsigned long cs, eip, eflags;
+        unsigned long sel, eip, eflags;
         uint32_t mask = EFLG_VIP | EFLG_VIF | EFLG_VM;
         if ( !mode_ring0() )
             mask |= EFLG_IOPL;
@@ -2797,7 +2827,7 @@ x86_emulate(
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &eip, op_bytes, ctxt, ops)) ||
              (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
-                              &cs, op_bytes, ctxt, ops)) ||
+                              &sel, op_bytes, ctxt, ops)) ||
              (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &eflags, op_bytes, ctxt, ops)) )
             goto done;
@@ -2807,7 +2837,8 @@ x86_emulate(
         _regs.eflags &= mask;
         _regs.eflags |= (uint32_t)(eflags & ~mask) | 0x02;
         _regs.eip = eip;
-        if ( (rc = load_seg(x86_seg_cs, cs, 1, ctxt, ops)) != 0 )
+        if ( (rc = load_seg(x86_seg_cs, sel, 1, &cs, ctxt, ops)) ||
+             (rc = commit_far_branch(&cs, eip)) )
             goto done;
         break;
     }
@@ -3437,7 +3468,8 @@ x86_emulate(
         generate_exception_if(mode_64bit(), EXC_UD, -1);
         eip = insn_fetch_bytes(op_bytes);
         sel = insn_fetch_type(uint16_t);
-        if ( (rc = load_seg(x86_seg_cs, sel, 0, ctxt, ops)) != 0 )
+        if ( (rc = load_seg(x86_seg_cs, sel, 0, &cs, ctxt, ops)) ||
+             (rc = commit_far_branch(&cs, eip)) )
             goto done;
         _regs.eip = eip;
         break;
@@ -3707,10 +3739,14 @@ x86_emulate(
             break;
         case 2: /* call (near) */
             dst.val = _regs.eip;
+            if ( (rc = ops->insn_fetch(x86_seg_cs, src.val, NULL, 0, ctxt)) )
+                goto done;
             _regs.eip = src.val;
             src.val = dst.val;
             goto push;
         case 4: /* jmp (near) */
+            if ( (rc = ops->insn_fetch(x86_seg_cs, src.val, NULL, 0, ctxt)) )
+                goto done;
             _regs.eip = src.val;
             dst.type = OP_NONE;
             break;
@@ -3729,14 +3765,17 @@ x86_emulate(
                 struct segment_register reg;
                 fail_if(ops->read_segment == NULL);
                 if ( (rc = ops->read_segment(x86_seg_cs, &reg, ctxt)) ||
-                     (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
+                     (rc = load_seg(x86_seg_cs, sel, 0, &cs, ctxt, ops)) ||
+                     (validate_far_branch(&cs, src.val),
+                      rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
                                       &reg.sel, op_bytes, ctxt)) ||
                      (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
-                                      &_regs.eip, op_bytes, ctxt)) )
+                                      &_regs.eip, op_bytes, ctxt)) ||
+                     (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) )
                     goto done;
             }
-
-            if ( (rc = load_seg(x86_seg_cs, sel, 0, ctxt, ops)) != 0 )
+            else if ( (rc = load_seg(x86_seg_cs, sel, 0, &cs, ctxt, ops)) ||
+                      (rc = commit_far_branch(&cs, src.val)) )
                 goto done;
             _regs.eip = src.val;
 
@@ -3818,7 +3857,7 @@ x86_emulate(
         generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1);
         generate_exception_if(!mode_ring0(), EXC_GP, 0);
         if ( (rc = load_seg((modrm_reg & 1) ? x86_seg_tr : x86_seg_ldtr,
-                            src.val, 0, ctxt, ops)) != 0 )
+                            src.val, 0, NULL, ctxt, ops)) != 0 )
             goto done;
         break;
 
@@ -4222,6 +4261,9 @@ x86_emulate(
             goto done;
 
         generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
+        generate_exception_if(user64 && (!is_canonical_address(_regs.edx) ||
+                                         !is_canonical_address(_regs.ecx)),
+                              EXC_GP, 0);
 
         cs.sel = (msr_content | 3) + /* SELECTOR_RPL_MASK */
                  (user64 ? 32 : 16);