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 B26C6385DC12 for ; Mon, 20 Apr 2020 07:34:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B26C6385DC12 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 11D40ACD0; Mon, 20 Apr 2020 07:34:34 +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> <7a1737f4-2371-e1c2-e1ce-1c35a76292a2@suse.com> From: Jan Beulich Message-ID: Date: Mon, 20 Apr 2020 09:34:33 +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=-11.7 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: Mon, 20 Apr 2020 07:34:37 -0000 On 20.04.2020 09:20, Hongtao Liu wrote: > On Thu, Apr 16, 2020 at 4:33 PM Jan Beulich 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. >>> @@ -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. > > I think yes. Please explain yourself. 66 48 C3 accesses 8 bytes of memory after all, whereas you'd generate a 2-byte access ahead of the LFENCE afaics. >>> @@ -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? > > Do you means add -mlfence-before-ret=yes which indicates > -mlfence-before-ret=shl? Yes. > Update my patch: I've not looked at this just yet - the issues above should be resolved first (one way or another). Jan