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 EC3B9384B0C1 for ; Tue, 21 Apr 2020 06:30:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EC3B9384B0C1 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 15993AA55; Tue, 21 Apr 2020 06:30:21 +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: <3c0f0998-23f8-12ad-f095-27d4f7173b16@suse.com> Date: Tue, 21 Apr 2020 08:30:16 +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.4 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: Tue, 21 Apr 2020 06:30:25 -0000 On 21.04.2020 04:24, Hongtao Liu wrote: > On Mon, Apr 20, 2020 at 3:34 PM Jan Beulich wrote: >> >> 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. >> > > 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? > @@ -647,7 +656,8 @@ static enum lfence_before_ret_kind > { > lfence_before_ret_none = 0, > lfence_before_ret_not, > - lfence_before_ret_or > + lfence_before_ret_or, > + lfence_before_ret_shl > } > lfence_before_ret; > > @@ -4350,22 +4360,28 @@ load_insn_p (void) > > if (!any_vex_p) > { > - /* lea */ > - if (i.tm.base_opcode == 0x8d) > + /* Anysize insns: lea, invlpg, clflush, prefetchnta, prefetcht0, > + prefetcht1, prefetcht2, prefetchtw, bndmk, bndcl, bndcu, bndcn, > + bndstx, bndldx, prefetchwt1, clflushopt, clwb, cldemote. */ Bad indentation (also elsewhere, so this may be an issue with your mail client)? > @@ -4536,8 +4577,8 @@ insert_lfence_before (void) > > if (i.reg_operands == 1) > { > - /* Indirect branch via register. Don't insert lfence with > - -mlfence-after-load=yes. */ > + /* Indirect branch via register. Insert lfence when > + -mlfence-after-load=none. */ > if (lfence_after_load > || lfence_before_indirect_branch == lfence_branch_memory) > return; The changed comment is awkward to read - the reader will almost certainly wonder why "none" implies an action. I think you either want to explain this further, or revert back to the original form by simply making it "... with -mlfence-after-load={all,general}." > @@ -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,50 @@ 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] && !(i.prefix[REX_PREFIX] & REX_W) > + ? 0x66 : flag_code == CODE_64BIT ? 0x48 : 0x0; While this now looks better, it's tailored to near RET. Far RET as well as IRET default to 32-bit operand size in 64-bit mode. I can't tell how relevant it is to match effective operand size of the guarded and guarding insns. > + 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; > + /* notl: 0xf71424, may add prefix > + for operand size overwrite or 64-bit code. */ Despite the comment extension you still say "notl". Please switch toi either just "not" or something like "not{w,l,q}". Also s/overwrite/override/. Note how you ... > + p = frag_more ((prefix ? 2 : 0) + 6 + 3); > + if (prefix) > + *p++ = prefix; > *p++ = 0xf7; > *p++ = 0x14; > *p++ = 0x24; > - /* notl: 0xf71424. */ > - if (flag_code == CODE_64BIT) > - *p++ = 0x48; > + if (prefix) > + *p++ = prefix; > *p++ = 0xf7; > *p++ = 0x14; > *p++ = 0x24; > } > + else > + { > + p = frag_more ((prefix ? 1 : 0) + 4 + 3); > + if (prefix) > + *p++ = prefix; > + if (lfence_before_ret == lfence_before_ret_or) > + { > + /* orl: 0x830c2400, may add prefix > + for operand size overwrite or 64-bit code. */ ... also have the same (bogus) suffixe here, but ... > + *p++ = 0x83; > + *p++ = 0x0c; > + } > + else > + { > + /* shl: 0xc1242400, may add prefix > + for operand size overwrite or 64-bit code. */ ... not here. Jan