[PATCH 1/2] i386: Generate lfence with load/indirect branch/ret [CVE-2020-0551]
Jan Beulich
jbeulich@suse.com
Thu Apr 16 08:33:02 GMT 2020
On 16.04.2020 07:34, Hongtao Liu wrote:
> I tried to re-arranged to use a common pattern (memory operand is
> destination) and only exclude those which don't also read this
> operand. But it turn out there still a lot of such instructions
> include all mov instruction, store instruction for i387 and cet,
> extract instructions, vgather instructions, vscatter instrcutions,
> convert instrcutions and so on, so i didn't re-arrange them.
> Other requests are done by the updated patch, also plus handling REP
> CMP/SCAS specially since they would set EFLAGS which affects control
> flow behavior.
>
> 1. No load for INVPCID, Implict load for POPS/POPF/POPA/XLATB
Why INVPCID? Whether it accesses its memory operand depends on
the value in the register operand. And what's POPS?
> 2. Add -mlfence-before-ret=shl, adjust operand size of or/not/shl to
> ret's.
> 3. Ajust -mlfence-after-load=[yes/no] to
> -mlfence-after-load=[none|general|all]. -mlfence-after-load=[none/all]
> equal original -mlfence-after-load=[no/yes],
While there wasn't any official release with the prior option forms
yet, I'm not sure it is a good idea to disallow the old forms
altogether now; they may need deprecating but still permitting
instead.
> -mlfence-after-load=general won't add lfence after REP CMPS/SCAS
> since they would affect control flow behavior.
> -mlfence-after-load=all will issue an warning when adding lfence
> after REP CMPS/SCAS.
I also think the various independent behavioral changes here would
better be split into separate patches (e.g. at least one patch per
numbered item in your enumeration above).
> 4. Adjust testcases and documents.
>
> gas/Changelog:
> * config/tc-i386.c (lfence_after_load_kine): New.
> (lfence_before_ret_shl): Change from lfence_before_ret_not.
> (load_insn_p): No load for INVPCID, implict load for
> POPS/POPA/POPF/XLATB.
> (insert_after_load): Insert lfence under
> -mlfence-after-load=[general|all],issue an warning when encounter
> REP CMPS/SCAS.
> (insert_before_before): Replace -mlfence-before-ret=not to
> -mlfence-before-ret=shl.
> (md_parse_option): Adjust -mlfence-after-load=[yes|no] to
> -mlfence-after-load=[none|general|all], Replace
> -mlfence-before-ret=not to -mlfence-before-ret=shl. Enable
> -mlfence-before-ret=shl when
> -mlfence-beofre-indirect-branch=all.
> (md_show_usage): Ditto.
> * doc/c-i386.texi: Ditto.
> * testsuite/gas/i386/i386.exp: Add new testcases.
> * gas/testsuite/gas/i386/lfence-load-b.d: New.
> * gas/testsuite/gas/i386/lfence-load-b.e: New.
> * gas/testsuite/gas/i386/lfence-load.d: Modified.
> * gas/testsuite/gas/i386/lfence-load.e: New.
> * gas/testsuite/gas/i386/lfence-load.s: Modified.
> * gas/testsuite/gas/i386/lfence-ret-a.d: Modified.
> * gas/testsuite/gas/i386/lfence-ret-b.d: Modified.
> * gas/testsuite/gas/i386/lfence-ret-c.d: New.
> * gas/testsuite/gas/i386/lfence-ret-d.d: New.
> * gas/testsuite/gas/i386/lfence-ret.s: Modified
> * gas/testsuite/gas/i386/x86-64-lfence-load-b.d: New.
> * gas/testsuite/gas/i386/x86-64-lfence-load.d: Modified.
> * gas/testsuite/gas/i386/x86-64-lfence-load.s: Modified.
> * gas/testsuite/gas/i386/x86-64-lfence-ret-a.d: Modified.
> * gas/testsuite/gas/i386/x86-64-lfence-ret-b.d: Modified.
> * gas/testsuite/gas/i386/x86-64-lfence-ret-c.d: New.
> * gas/testsuite/gas/i386/x86-64-lfence-ret-d.d: New.
There's a stray leading gas/ on the last so many lines above.
Also could you please send patches inline, unless they're too
big to be permitted by list restrictions? Commenting on an
attachment is quite a bit more cumbersome. Anyway, I'll try to.
>-/* 1 if lfence should be inserted after every load. */
>-static int lfence_after_load = 0;
>+/* Non-zero if lfence shoulde be inserted after load. */
Please try to avoid breaking correct spelling ("should"). I
also think the comment should briefly explain the difference
between lfence_load_general and lfence_load_all, even if
this may seem redundant with the command line option doc.
>@@ -4350,21 +4357,28 @@ load_insn_p (void)
>
> if (!any_vex_p)
> {
>- /* lea */
>- if (i.tm.base_opcode == 0x8d)
>+ /* Note: invlpg, invpcid, clflush, clflushopt, prefetchh, prefetchw
>+ could be excluded by the later pattern. */
>+ /* lea, invpcid. */
>+ if (i.tm.base_opcode == 0x8d
>+ || i.tm.base_opcode == 0xf3882)
The first comment mentions INVPCID, but the second does, too,
which is not logical. Also what about CLDEMOTE or CLWB, just
to name a few examples not listed? Instead of relying on
later patterns, could you perhaps bail for all AnySize insns
here?
>- /* pop */
>- if ((i.tm.base_opcode & ~7) == 0x58
>- || (i.tm.base_opcode == 0x8f && i.tm.extension_opcode == 0))
>+ /* pop, popf, popa. */
>+ if (strcmp (i.tm.name, "pop") == 0
>+ || i.tm.base_opcode == 0x9d
>+ || i.tm.base_opcode == 0x61)
Personally I'd recommend against string matching, and even
more so against a mixture of it and opcode matching. But I'm
not the maintainer of this code.
>- /* outs */
>- if (base_opcode == 0x6f)
>+ /* NB: For AMD-specific insns with implicit memory operands,
>+ they're intentionally not covered.
>+ outs, xlatb. */
>+ if (base_opcode == 0x6f
>+ || i.tm.base_opcode == 0xD7)
> return 1;
I'd like to request consistency in choice of case in numeric
(hex) constant. I'd also think the AMD part of the comment
would better go after this if()+return.
While RET/LRET get handled specially anyway, what about e.g.
IRET which also loads data from memory?
>@@ -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.
>+ {
>+ if (lfence_after_load == lfence_load_general)
>+ {
>+ as_warn (_("`%s` skips -mlfence-after-general=general"),
Mis-spelled option name?
>@@ -4583,33 +4613,47 @@ 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;
>- }
>- else
>+
>+ char prefix = i.prefix[DATA_PREFIX] ? 0x66
>+ : flag_code == CODE_64BIT ? 0x48 : 0x0;
Is this correct when the RET _also_ has an explicitly specified
REX.W prefix? Also indentation looks somewhat odd on the last
line of this block.
>+
>+ if (lfence_before_ret == lfence_before_ret_not)
> {
>- p = frag_more ((flag_code == CODE_64BIT ? 2 : 0) + 6 + 3);
> /* notl: 0xf71424. */
Comments like this one are no longer precise: The l suffix is
generally wrong for 64-bit code, and would also be wrong if
there was an operand size override on the RET.
> case OPTION_MLFENCE_BEFORE_INDIRECT_BRANCH:
> if (strcasecmp (arg, "all") == 0)
>- lfence_before_indirect_branch = lfence_branch_all;
>+ {
>+ lfence_before_indirect_branch = lfence_branch_all;
>+ lfence_before_ret = lfence_before_ret_shl;
>+ }
I don't think this should override an earlier explicit
-mlfence-before-ret= (i.e. in particular the order the two
options would be specified in should imo not matter).
>@@ -13012,6 +13061,8 @@ md_parse_option (int c, const char *arg)
> lfence_before_ret = lfence_before_ret_or;
> else if (strcasecmp (arg, "not") == 0)
> lfence_before_ret = lfence_before_ret_not;
>+ else if (strcasecmp (arg, "shl") == 0)
>+ lfence_before_ret = lfence_before_ret_shl;
> else if (strcasecmp (arg, "none") == 0)
> lfence_before_ret = lfence_before_ret_none;
> else
With the SHL variant being truly benign (except for the
performance impact of course), would it make sense to also
allow for a simple "=yes" form now?
Jan
More information about the Binutils
mailing list