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

[Hongtao Liu]

On Wed, Apr 22, 2020 at 4:47 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.04.2020 05:33, Hongtao Liu wrote:
> > On Tue, Apr 21, 2020 at 2:30 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 21.04.2020 04:24, Hongtao Liu wrote:
> >>> On Mon, Apr 20, 2020 at 3:34 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 20.04.2020 09:20, Hongtao Liu wrote:
> >>>>> On Thu, Apr 16, 2020 at 4:33 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>> On 16.04.2020 07:34, Hongtao Liu wrote:
> >>>>>>> @@ -4506,6 +4520,22 @@ insert_lfence_after (void)
> >>>>>>> {
> >>>>>>>   if (lfence_after_load && load_insn_p ())
> >>>>>>>     {
> >>>>>>> +      /* Insert lfence after rep cmps/scas only under
> >>>>>>> +       -mlfence-after-load=all.  */
> >>>>>>> +      if (((i.tm.base_opcode | 0x1) == 0xa7
> >>>>>>> +         || (i.tm.base_opcode | 0x1) == 0xaf)
> >>>>>>> +        && i.prefix[REP_PREFIX])
> >>>>>>
> >>>>>> I'm afraid I don't understand why the REP forms need treating
> >>>>>> differently from the non-REP ones of the same insns.
> >>>>>>
> >>>>>
> >>>>> Not all REP forms, just REP CMPS/SCAS which would change EFLAGS.
> >>>>
> >>>> Well, of course just the two. But this doesn't answer my question
> >>>> as to why there is such a special case.
> >>>>
> >>>
> >>> There are also two REP string instructions that require special
> >>> treatment. Specifically, the compare string (CMPS) and scan string
> >>> (SCAS) instructions set EFLAGS in a manner that depends on the data
> >>> being compared/scanned. When used with a REP prefix, the number of
> >>> iterations may therefore vary depending on this data. If the data is a
> >>> program secret chosen by the adversary using an LVI method, then this
> >>> data-dependent behavior may leak some aspect of the secret. The
> >>> solution is to unfold any REP CMPS and REP SCAS operations into a loop
> >>> and insert an LFENCE after the CMPS/SCAS instruction. For example,
> >>> REPNZ SCAS can be unfolded to:
> >>>
> >>> .RepLoop:
> >>>   JRCXZ .ExitRepLoop
> >>>   DEC rcx  # or ecx if the REPNZ SCAS uses a 32-bit address size
> >>>   SCAS
> >>>   LFENCE
> >>>   JNZ .RepLoop
> >>> .ExitRepLoop:
> >>>   ...
> >>>
> >>> The request i get is to add options to handle or not handle REP
> >>> CMPS/SCAS also plus issue a warning.
> >>
> >> But you don't handle them as per what you've written above, afaics.
> >> Am I overlooking anything?
> >
> > Well, that solution is not meant for gas, i put them here for
> > convienence of understanding of why we need to handle REP CMPS/SCAS
> > specially.
>
> And how is it better then to issue a warning and leave the code
> alone over still at least inserting an LFENCE after the insn? I.e.
> I'm not sure I see the value of the separate "general" and "all"
> sub-options then.

You're right, i'll revert sub-options for lfence_after_load, and only
issue a warning for REP CMPS/SCAS.

>
> As to it not being meant for gas - why is that?
>

And handle REP CMPS/SCAS stuff in another separate patch.

> > @@ -4568,12 +4609,13 @@ insert_lfence_before (void)
> >        return;
> >      }
> >
> > -  /* Output or/not and lfence before ret.  */
> > +  /* Output or/not/shl and lfence before ret/lret/iret.  */
> >    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))
> > +          || i.tm.base_opcode == 0xcb
> > +          || i.tm.base_opcode == 0xcf))
> >      {
> >        if (last_insn.kind != last_insn_other
> >            && last_insn.seg == now_seg)
> > @@ -4583,33 +4625,59 @@ insert_lfence_before (void)
> >                           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;
> > -          *p++ = 0x83;
> > -          *p++ = 0xc;
> > -          *p++ = 0x24;
> > -          *p++ = 0x0;
> > -        }
> > +
> > +      bfd_boolean lret = (i.tm.base_opcode | 0x1) == 0xcb;
>
> "(i.tm.base_opcode | 0x5) == 0xcf" or "(i.tm.base_opcode & 8)"
> to also cover IRET.
>

Changed.

> > +      bfd_boolean has_rexw = i.prefix[REX_PREFIX] & REX_W;
> > +      char prefix = 0x0;
> > +      /* Default operand size for far return is 32 bits,
> > +         64 bits for near return.  */
> > +      if (has_rexw)
> > +        prefix = 0x48;
> >        else
> > +        prefix = i.prefix[DATA_PREFIX]
> > +                 ? 0x66
> > +                 : !lret && flag_code == CODE_64BIT ? 0x48 : 0x0;
>
> Aiui the workaround is specifically for Intel CPUs. Intel CPUs
> ignore operand size overrides on near RET. (Sorry, I should
> have pointed out this fact earlier already.)

I don't quite understand your point, could you give a testcase to show that?

>
> > +      if (lfence_before_ret == lfence_before_ret_not)
> >          {
> > -          p = frag_more ((flag_code == CODE_64BIT ? 2 : 0) + 6 + 3);
> > -          /* notl: 0xf71424.  */
> > -          if (flag_code == CODE_64BIT)
> > -            *p++ = 0x48;
> > +          /* not: 0xf71424, may add prefix
> > +             for operand size overwrite or 64-bit code.  */
>
> As said before - "override", not "overwrite" (there are several
> instances to change).

Changed.

>
> Jan

-- 
BR,
Hongtao