From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 8D0FF384A01E for ; Thu, 16 Apr 2020 08:33:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8D0FF384A01E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=jbeulich@suse.com X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id F2482AEA6; Thu, 16 Apr 2020 08:33:07 +0000 (UTC) Subject: Re: [PATCH 1/2] i386: Generate lfence with load/indirect branch/ret [CVE-2020-0551] To: Hongtao Liu Cc: "H.J. Lu" , Binutils , "Lu, Hongjiu" References: <20200310160528.303613-1-hjl.tools@gmail.com> <20200310160528.303613-2-hjl.tools@gmail.com> <6dcb50a3-d3ad-6abc-8a5f-703373df95a1@suse.com> <70acc1ec-ce95-9df7-cdce-1d6c2305b1b4@suse.com> From: Jan Beulich Message-ID: <7a1737f4-2371-e1c2-e1ce-1c35a76292a2@suse.com> Date: Thu, 16 Apr 2020 10:33:02 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Apr 2020 08:33:11 -0000 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