| Message ID | 57EB99320200007800113213@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/09/16 09:19, Jan Beulich wrote: > Especially for x86_insn_operand_ea() to return dependable segment > information even when the caller didn't consider applicability, we > shouldn't have ea.type start out as OP_MEM. Make it OP_NONE instead, > and set it to OP_MEM when we actually encounter memory like operands. > > This requires to eliminate the XSA-123 fix, which has been no longer > necessary since the elimination of the union in commit dd766684e7. That > in turn allows restricting the scope of override_seg to x86_decode(). > At this occasion also make it have a proper type, instead of plain int. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -1647,7 +1647,6 @@ struct x86_emulate_state { > opcode_desc_t desc; > union vex vex; > union evex evex; > - int override_seg; > > /* > * Data operand effective address (usually computed from ModRM). > @@ -1683,7 +1682,6 @@ struct x86_emulate_state { > #define lock_prefix (state->lock_prefix) > #define vex (state->vex) > #define evex (state->evex) > -#define override_seg (state->override_seg) > #define ea (state->ea) > > static int > @@ -1712,6 +1710,7 @@ x86_decode_onebyte( > case 0xa0: case 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */ > case 0xa2: case 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */ > /* Source EA is not encoded via ModRM. */ > + ea.type = OP_MEM; > ea.mem.off = insn_fetch_bytes(ad_bytes); > break; > > @@ -1802,11 +1801,11 @@ x86_decode( > { > uint8_t b, d, sib, sib_index, sib_base; > unsigned int def_op_bytes, def_ad_bytes, opcode; > + enum x86_segment override_seg = x86_seg_none; > int rc = X86EMUL_OKAY; > > memset(state, 0, sizeof(*state)); > - override_seg = -1; > - ea.type = OP_MEM; > + ea.type = OP_NONE; > ea.mem.seg = x86_seg_ds; > ea.reg = PTR_POISON; > state->regs = ctxt->regs; > @@ -2102,6 +2101,7 @@ x86_decode( > else if ( ad_bytes == 2 ) > { > /* 16-bit ModR/M decode. */ > + ea.type = OP_MEM; > switch ( modrm_rm ) > { > case 0: > @@ -2152,6 +2152,7 @@ x86_decode( > else > { > /* 32/64-bit ModR/M decode. */ > + ea.type = OP_MEM; > if ( modrm_rm == 4 ) > { > sib = insn_fetch_type(uint8_t); > @@ -2216,7 +2217,7 @@ x86_decode( > } > } > > - if ( override_seg != -1 && ea.type == OP_MEM ) > + if ( override_seg != x86_seg_none ) I don't see why the "ea.type == OP_MEM" should be dropped at this point. We have already set ea.type appropriately for memory instructions by this point, and it does open up the case where instructions which would have triggered XSA-123 get incorrect information reported if queried with x86_insn_operand_ea() ~Andrew > ea.mem.seg = override_seg; > > /* Fetch the immediate operand, if present. */ > @@ -4253,13 +4254,11 @@ x86_emulate( > generate_exception_if(limit < sizeof(long) || > (limit & (limit - 1)), EXC_UD, -1); > base &= ~(limit - 1); > - if ( override_seg == -1 ) > - override_seg = x86_seg_ds; > if ( ops->rep_stos ) > { > unsigned long nr_reps = limit / sizeof(zero); > > - rc = ops->rep_stos(&zero, override_seg, base, sizeof(zero), > + rc = ops->rep_stos(&zero, ea.mem.seg, base, sizeof(zero), > &nr_reps, ctxt); > if ( rc == X86EMUL_OKAY ) > { > @@ -4271,7 +4270,7 @@ x86_emulate( > } > while ( limit ) > { > - rc = ops->write(override_seg, base, &zero, sizeof(zero), ctxt); > + rc = ops->write(ea.mem.seg, base, &zero, sizeof(zero), ctxt); > if ( rc != X86EMUL_OKAY ) > goto done; > base += sizeof(zero); > @@ -5257,7 +5256,6 @@ x86_emulate( > #undef rex_prefix > #undef lock_prefix > #undef vex > -#undef override_seg > #undef ea > > #ifdef __XEN__ > > >
>>> On 29.09.16 at 23:12, <andrew.cooper3@citrix.com> wrote: > On 28/09/16 09:19, Jan Beulich wrote: >> @@ -2216,7 +2217,7 @@ x86_decode( >> } >> } >> >> - if ( override_seg != -1 && ea.type == OP_MEM ) >> + if ( override_seg != x86_seg_none ) > > I don't see why the "ea.type == OP_MEM" should be dropped at this > point. We have already set ea.type appropriately for memory > instructions by this point, and it does open up the case where > instructions which would have triggered XSA-123 get incorrect > information reported if queried with x86_insn_operand_ea() The need to remove this actually became apparent with the testing I did for the priv-op handling, namely for OUTS with a segment override: When we had (before the patch here) ea.type start out as OP_MEM, the conditional above was true _unless_ ea.type got changed later on. With it now (properly imo) starting out as OP_NONE, instructions not changing it to OP_MEM (like all the string ones) would not get the segment override applied anymore. And no, x86_insn_operand_ea() returns x86_seg_none when ea.type is anything other than OP_MEM. Jan
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1647,7 +1647,6 @@ struct x86_emulate_state { opcode_desc_t desc; union vex vex; union evex evex; - int override_seg; /* * Data operand effective address (usually computed from ModRM). @@ -1683,7 +1682,6 @@ struct x86_emulate_state { #define lock_prefix (state->lock_prefix) #define vex (state->vex) #define evex (state->evex) -#define override_seg (state->override_seg) #define ea (state->ea) static int @@ -1712,6 +1710,7 @@ x86_decode_onebyte( case 0xa0: case 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */ case 0xa2: case 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */ /* Source EA is not encoded via ModRM. */ + ea.type = OP_MEM; ea.mem.off = insn_fetch_bytes(ad_bytes); break; @@ -1802,11 +1801,11 @@ x86_decode( { uint8_t b, d, sib, sib_index, sib_base; unsigned int def_op_bytes, def_ad_bytes, opcode; + enum x86_segment override_seg = x86_seg_none; int rc = X86EMUL_OKAY; memset(state, 0, sizeof(*state)); - override_seg = -1; - ea.type = OP_MEM; + ea.type = OP_NONE; ea.mem.seg = x86_seg_ds; ea.reg = PTR_POISON; state->regs = ctxt->regs; @@ -2102,6 +2101,7 @@ x86_decode( else if ( ad_bytes == 2 ) { /* 16-bit ModR/M decode. */ + ea.type = OP_MEM; switch ( modrm_rm ) { case 0: @@ -2152,6 +2152,7 @@ x86_decode( else { /* 32/64-bit ModR/M decode. */ + ea.type = OP_MEM; if ( modrm_rm == 4 ) { sib = insn_fetch_type(uint8_t); @@ -2216,7 +2217,7 @@ x86_decode( } } - if ( override_seg != -1 && ea.type == OP_MEM ) + if ( override_seg != x86_seg_none ) ea.mem.seg = override_seg; /* Fetch the immediate operand, if present. */ @@ -4253,13 +4254,11 @@ x86_emulate( generate_exception_if(limit < sizeof(long) || (limit & (limit - 1)), EXC_UD, -1); base &= ~(limit - 1); - if ( override_seg == -1 ) - override_seg = x86_seg_ds; if ( ops->rep_stos ) { unsigned long nr_reps = limit / sizeof(zero); - rc = ops->rep_stos(&zero, override_seg, base, sizeof(zero), + rc = ops->rep_stos(&zero, ea.mem.seg, base, sizeof(zero), &nr_reps, ctxt); if ( rc == X86EMUL_OKAY ) { @@ -4271,7 +4270,7 @@ x86_emulate( } while ( limit ) { - rc = ops->write(override_seg, base, &zero, sizeof(zero), ctxt); + rc = ops->write(ea.mem.seg, base, &zero, sizeof(zero), ctxt); if ( rc != X86EMUL_OKAY ) goto done; base += sizeof(zero); @@ -5257,7 +5256,6 @@ x86_emulate( #undef rex_prefix #undef lock_prefix #undef vex -#undef override_seg #undef ea #ifdef __XEN__