[PATCH 1/2] i386: Generate lfence with load/indirect branch/ret [CVE-2020-0551]

[Jan Beulich]

On 10.03.2020 17:05, H.J. Lu wrote:
> @@ -4311,6 +4333,291 @@ optimize_encoding (void)
>      }
>  }
>  
> +/* Return non-zero for load instruction.  */
> +
> +static int
> +load_insn_p (void)
> +{
> +  unsigned int dest;
> +  int any_vex_p = is_any_vex_encoding (&i.tm);
> +
> +  if (!any_vex_p)
> +    {
> +      /* lea  */
> +      if (i.tm.base_opcode == 0x8d)
> +	return 0;

Also include INVLPG, CLFLUSH etc, and maybe some prefetches here?
(I'll mention the LEA-like MPX insns as well, but I think I can
predict your reply.)

> +      /* pop  */
> +      if ((i.tm.base_opcode & 0xfffffff8) == 0x58

Mind using ~7 instead?

> +	  || (i.tm.base_opcode == 0x8f && i.tm.extension_opcode == 0))
> +	return 1;

What about segment register POPs, POPF, POPA, ENTER, and LEAVE?

> +      /* movs, cmps, lods, scas.  */
> +      if ((i.tm.base_opcode >= 0xa4 && i.tm.base_opcode <= 0xa7)
> +	  || (i.tm.base_opcode >= 0xac && i.tm.base_opcode <= 0xaf))

This can be had with a single comparison:

      if ((i.tm.base_opcode | 0xb) == 0xaf)

> +	return 1;
> +
> +      /* outs */
> +      if (i.tm.base_opcode == 0x6e || i.tm.base_opcode == 0x6f)

And here:

      if ((i.tm.base_opcode | 1) == 0x6f)

Similar folding of comparisons may also be desirable further down.

Also, what about XLATB? What about implicit memory accesses done by
e.g. segment register loads? As to AMD-specific insns with implicit
memory operands (often accessed through rAX), should the doc
perhaps mention they're intentionally not covered?

> +	return 1;
> +    }
> +
> +  /* No memory operand.  */
> +  if (!i.mem_operands)
> +    return 0;
> +
> +  if (any_vex_p)
> +    {
> +      /* vldmxcsr.  */
> +      if (i.tm.base_opcode == 0xae
> +	  && i.tm.opcode_modifier.vex
> +	  && i.tm.opcode_modifier.vexopcode == VEX0F
> +	  && i.tm.extension_opcode == 2)
> +	return 1;
> +    }
> +  else
> +    {
> +      /* test, not, neg, mul, imul, div, idiv.  */
> +      if ((i.tm.base_opcode == 0xf6 || i.tm.base_opcode == 0xf7)
> +	  && i.tm.extension_opcode != 1)
> +	return 1;
> +
> +      /* inc, dec.  */
> +      if ((i.tm.base_opcode == 0xfe || i.tm.base_opcode == 0xff)
> +	  && i.tm.extension_opcode <= 1)
> +	return 1;
> +
> +      /* add, or, adc, sbb, and, sub, xor, cmp.  */
> +      if (i.tm.base_opcode >= 0x80 && i.tm.base_opcode <= 0x83)
> +	return 1;
> +
> +      /* bt, bts, btr, btc.  */
> +      if (i.tm.base_opcode == 0xfba
> +	  && (i.tm.extension_opcode >= 4 && i.tm.extension_opcode <= 7))
> +	return 1;
> +
> +      /* rol, ror, rcl, rcr, shl/sal, shr, sar. */
> +      if ((i.tm.base_opcode == 0xc0
> +	   || i.tm.base_opcode == 0xc1
> +	   || (i.tm.base_opcode >= 0xd0 && i.tm.base_opcode <= 0xd3))
> +	  && i.tm.extension_opcode != 6)
> +	return 1;
> +
> +      /* cmpxchg8b, cmpxchg16b, xrstors.  */
> +      if (i.tm.base_opcode == 0xfc7
> +	  && (i.tm.extension_opcode == 1 || i.tm.extension_opcode == 3))
> +	return 1;
> +
> +      /* fxrstor, ldmxcsr, xrstor.  */
> +      if (i.tm.base_opcode == 0xfae
> +	  && (i.tm.extension_opcode == 1
> +	      || i.tm.extension_opcode == 2
> +	      || i.tm.extension_opcode == 5))
> +	return 1;
> +
> +      /* lgdt, lidt, lmsw.  */
> +      if (i.tm.base_opcode == 0xf01
> +	  && (i.tm.extension_opcode == 2
> +	      || i.tm.extension_opcode == 3
> +	      || i.tm.extension_opcode == 6))
> +	return 1;
> +
> +      /* vmptrld */
> +      if (i.tm.base_opcode == 0xfc7
> +	  && i.tm.extension_opcode == 6)
> +	return 1;
> +
> +      /* Check for x87 instructions.  */
> +      if (i.tm.base_opcode >= 0xd8 && i.tm.base_opcode <= 0xdf)
> +	{
> +	  /* Skip fst, fstp, fstenv, fstcw.  */
> +	  if (i.tm.base_opcode == 0xd9
> +	      && (i.tm.extension_opcode == 2
> +		  || i.tm.extension_opcode == 3
> +		  || i.tm.extension_opcode == 6
> +		  || i.tm.extension_opcode == 7))
> +	    return 0;
> +
> +	  /* Skip fisttp, fist, fistp, fstp.  */
> +	  if (i.tm.base_opcode == 0xdb
> +	      && (i.tm.extension_opcode == 1
> +		  || i.tm.extension_opcode == 2
> +		  || i.tm.extension_opcode == 3
> +		  || i.tm.extension_opcode == 7))
> +	    return 0;
> +
> +	  /* Skip fisttp, fst, fstp, fsave, fstsw.  */
> +	  if (i.tm.base_opcode == 0xdd
> +	      && (i.tm.extension_opcode == 1
> +		  || i.tm.extension_opcode == 2
> +		  || i.tm.extension_opcode == 3
> +		  || i.tm.extension_opcode == 6
> +		  || i.tm.extension_opcode == 7))
> +	    return 0;
> +
> +	  /* Skip fisttp, fist, fistp, fbstp, fistp.  */
> +	  if (i.tm.base_opcode == 0xdf
> +	      && (i.tm.extension_opcode == 1
> +		  || i.tm.extension_opcode == 2
> +		  || i.tm.extension_opcode == 3
> +		  || i.tm.extension_opcode == 6
> +		  || i.tm.extension_opcode == 7))
> +	    return 0;
> +
> +	  return 1;
> +	}
> +    }
> +
> +  dest = i.operands - 1;
> +
> +  /* Check fake imm8 operand and 3 source operands.  */
> +  if ((i.tm.opcode_modifier.immext
> +       || i.tm.opcode_modifier.vexsources == VEX3SOURCES)
> +      && i.types[dest].bitfield.imm8)
> +    dest--;
> +
> +  /* add, or, adc, sbb, and, sub, xor, cmp, test, xchg, xadd  */
> +  if (!any_vex_p
> +      && (i.tm.base_opcode == 0x0
> +	  || i.tm.base_opcode == 0x1
> +	  || i.tm.base_opcode == 0x8
> +	  || i.tm.base_opcode == 0x9
> +	  || i.tm.base_opcode == 0x10
> +	  || i.tm.base_opcode == 0x11
> +	  || i.tm.base_opcode == 0x18
> +	  || i.tm.base_opcode == 0x19
> +	  || i.tm.base_opcode == 0x20
> +	  || i.tm.base_opcode == 0x21
> +	  || i.tm.base_opcode == 0x28
> +	  || i.tm.base_opcode == 0x29
> +	  || i.tm.base_opcode == 0x30
> +	  || i.tm.base_opcode == 0x31
> +	  || i.tm.base_opcode == 0x38
> +	  || i.tm.base_opcode == 0x39
> +	  || (i.tm.base_opcode >= 0x84 && i.tm.base_opcode <= 0x87)
> +	  || i.tm.base_opcode == 0xfc0
> +	  || i.tm.base_opcode == 0xfc1))
> +    return 1;

Don't quite a few of these fit very well with ...

> +  /* Check for load instruction.  */
> +  return (i.types[dest].bitfield.class != ClassNone
> +	  || i.types[dest].bitfield.instance == Accum);

... this generic expression? It would seem to me that only TEST
and XCHG need special casing, for allowing either operand order.
Same seems to apply to quite a few of the special cases in the
big "else" block further up, and even its if() [vldmxcsr] part.

> +static void
> +insert_lfence_before (void)
> +{
> +  char *p;
> +
> +  if (i.tm.base_opcode == 0xff
> +      && (i.tm.extension_opcode == 2 || i.tm.extension_opcode == 4))

Also exclude VEX- and alike encoded insn here again?

> +    {
> +      /* Insert lfence before indirect branch if needed.  */
> +
> +      if (lfence_before_indirect_branch == lfence_branch_none)
> +	return;
> +
> +      if (i.operands != 1)
> +	abort ();
> +
> +      if (i.reg_operands == 1)
> +	{
> +	  /* Indirect branch via register.  Don't insert lfence with
> +	     -mlfence-after-load=yes.  */
> +	  if (lfence_after_load
> +	      || lfence_before_indirect_branch == lfence_branch_memory)
> +	    return;
> +	}
> +      else if (i.mem_operands == 1
> +	       && lfence_before_indirect_branch != lfence_branch_register)
> +	{
> +	  as_warn (_("indirect branch `%s` over memory should be avoided"),
> +		   i.tm.name);

Perhaps drop "branch" and replace "over memory" by "with memory operand"?

> +	  return;
> +	}
> +      else
> +	return;
> +
> +      if (last_insn.kind != last_insn_other
> +	  && last_insn.seg == now_seg)
> +	{
> +	  as_warn_where (last_insn.file, last_insn.line,
> +			 _("`%s` skips -mlfence-before-indirect-branch on `%s`"),
> +			 last_insn.name, i.tm.name);
> +	  return;
> +	}
> +
> +      p = frag_more (3);
> +      *p++ = 0xf;
> +      *p++ = 0xae;
> +      *p = 0xe8;
> +      return;
> +    }
> +
> +  /* Output orl/notl and lfence before ret.  */

May I suggest to either drop the insn suffixes here (and below),
or make them correctly reflect the code below (which may also
produce q- or w-suffixed insns)?

> +  if (lfence_before_ret != lfence_before_ret_none
> +      && (i.tm.base_opcode == 0xc2
> +	  || i.tm.base_opcode == 0xc3
> +	  || i.tm.base_opcode == 0xca
> +	  || i.tm.base_opcode == 0xcb))
> +    {
> +      if (last_insn.kind != last_insn_other
> +	  && last_insn.seg == now_seg)
> +	{
> +	  as_warn_where (last_insn.file, last_insn.line,
> +			 _("`%s` skips -mlfence-before-ret on `%s`"),
> +			 last_insn.name, i.tm.name);
> +	  return;
> +	}
> +      if (lfence_before_ret == lfence_before_ret_or)
> +	{
> +	  /* orl: 0x830c2400.  */
> +	  p = frag_more ((flag_code == CODE_64BIT ? 1 : 0) + 4 + 3);
> +	  if (flag_code == CODE_64BIT)
> +	    *p++ = 0x48;

Shouldn't this depend on RET's operand size? Likewise wouldn't you
also need to insert 0x66/0x67 in certain cases?

> +	  *p++ = 0x83;
> +	  *p++ = 0xc;
> +	  *p++ = 0x24;
> +	  *p++ = 0x0;
> +	}
> +      else
> +	{
> +	  p = frag_more ((flag_code == CODE_64BIT ? 2 : 0) + 6 + 3);
> +	  /* notl: 0xf71424.  */
> +	  if (flag_code == CODE_64BIT)
> +	    *p++ = 0x48;
> +	  *p++ = 0xf7;
> +	  *p++ = 0x14;
> +	  *p++ = 0x24;
> +	  if (flag_code == CODE_64BIT)
> +	    *p++ = 0x48;
> +	  /* notl: 0xf71424.  */
> +	  *p++ = 0xf7;
> +	  *p++ = 0x14;
> +	  *p++ = 0x24;

When reading the description I was wondering about the use of NOT.
I think the doc should mention that it's _two_ NOTs that get inserted,
as this is even more growth of code size than the OR variant. Is
there a performance reason for having this extra, more expensive (in
terms of code size) variant? Or is it rather because of the OR
variant clobbering EFLAGS (which ought to be called out in the doc)?
In which case - was it considered to use e.g. SHL with an immediate
of zero, thus having smaller code _and_ untouched EFLAGS (but of
course requiring at least an 80186, albeit the addressing mode
used requires a 386 anyway, which you don't seem to be checking
anywhere)?

Also I guess the last comment above would better move two lines up?

> @@ -12668,6 +12986,41 @@ md_parse_option (int c, const char *arg)
>          as_fatal (_("invalid -mfence-as-lock-add= option: `%s'"), arg);
>        break;
>  
> +    case OPTION_MLFENCE_AFTER_LOAD:
> +      if (strcasecmp (arg, "yes") == 0)
> +	lfence_after_load = 1;
> +      else if (strcasecmp (arg, "no") == 0)
> +	lfence_after_load = 0;
> +      else
> +        as_fatal (_("invalid -mlfence-after-load= option: `%s'"), arg);
> +      break;
> +
> +    case OPTION_MLFENCE_BEFORE_INDIRECT_BRANCH:
> +      if (strcasecmp (arg, "all") == 0)
> +	lfence_before_indirect_branch = lfence_branch_all;

I wonder whether this shouldn't also enable a safe lfence_before_ret
mode (i.e. not the OR one), for RET also being an indirect branch. Of
course care would need to be taken to avoid clobbering an already set
lfence_before_ret mode.

> @@ -13254,6 +13616,10 @@ i386_cons_align (int ignore ATTRIBUTE_UNUSED)
>        last_insn.kind = last_insn_directive;
>        last_insn.name = "constant directive";
>        last_insn.file = as_where (&last_insn.line);
> +      if (lfence_before_ret != lfence_before_ret_none)
> +	as_warn (_("constant directive skips -mlfence-before-ret"));
> +      if (lfence_before_indirect_branch != lfence_branch_none)
> +	as_warn (_("constant directive skips -mlfence-before-indirect-branch"));

Could these be folded into a single warning, to avoid getting overly
verbose?

Jan