Re: [Xen-devel] [PATCH v2 16/16] x86emul: don't assume a memory operand

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>
> --- 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__
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel