|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Ping: [PATCH 2/5] x86emul: limit-check branch targets
>>> On 17.02.16 at 17:35, <JBeulich@xxxxxxxx> wrote:
> 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@xxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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
>
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -745,7 +745,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;
> if ( ((access_type != hvm_access_insn_fetch
> ? vio->mmio_access.read_access
> @@ -811,13 +811,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);
> @@ -849,7 +853,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;
>
> if ( vio->mmio_access.write_access &&
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3685,7 +3685,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;
> }
> @@ -3709,7 +3709,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 )
> @@ -3740,7 +3740,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;
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -157,7 +157,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 )
> @@ -241,7 +241,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(
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5126,10 +5126,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;
> @@ -5278,7 +5279,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);
> @@ -5394,7 +5395,8 @@ int mmio_ro_emulated_write(
> struct mmio_ro_emulate_ctxt *mmio_ro_ctxt = ctxt->data;
>
> /* 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);
> @@ -5423,7 +5425,7 @@ int mmcfg_intercept_write(
> * Only allow naturally-aligned stores no wider than 4 bytes to the
> * original %cr2 address.
> */
> - if ( ((bytes | offset) & (bytes - 1)) || bytes > 4 ||
> + if ( ((bytes | offset) & (bytes - 1)) || bytes > 4 || !bytes ||
> offset != mmio_ctxt->cr2 )
> {
> MEM_LOG("mmcfg_intercept: bad write (cr2=%lx, addr=%lx, bytes=%u)",
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -642,14 +642,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;
> @@ -1099,29 +1111,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;
> + int rc = ops->read_segment(seg, sreg, ctxt);
>
> - if ( (rc = ops->read_segment(seg, ®, ctxt)) != 0 )
> - return rc;
> -
> - 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, ®, 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;
> @@ -1132,8 +1145,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. */
> @@ -1242,16 +1255,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 )
> @@ -1265,17 +1278,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 = ®
> +
> 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 == ® )
> + rc = ops->write_segment(seg, sreg, ctxt);
>
> - return realmode_load_seg(seg, sel, ctxt, ops);
> + return rc;
> }
>
> void *
> @@ -1970,6 +1995,8 @@ x86_emulate(
>
> switch ( b )
> {
> + struct segment_register cs;
> +
> case 0x00 ... 0x05: add: /* add */
> emulate_2op_SrcV("add", src, dst, _regs.eflags);
> break;
> @@ -2031,7 +2058,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;
>
> @@ -2364,7 +2391,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;
> @@ -2439,14 +2466,15 @@ x86_emulate(
> sel = insn_fetch_type(uint16_t);
>
> if ( (rc = ops->read_segment(x86_seg_cs, ®, 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),
> ®.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;
> }
> @@ -2664,7 +2692,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;
> @@ -2679,7 +2708,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;
> @@ -2753,7 +2782,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;
> @@ -2782,7 +2812,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;
> @@ -2792,7 +2822,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;
> @@ -2802,7 +2832,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;
> }
> @@ -3432,7 +3463,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;
> @@ -3702,10 +3734,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;
> @@ -3724,14 +3760,17 @@ x86_emulate(
> struct segment_register reg;
> fail_if(ops->read_segment == NULL);
> if ( (rc = ops->read_segment(x86_seg_cs, ®, 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),
> ®.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;
>
> @@ -3816,7 +3855,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;
>
> @@ -4269,6 +4308,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);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |