From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C1A4B20945B9C for ; Thu, 16 Aug 2018 13:05:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0EAA9819703A; Thu, 16 Aug 2018 20:05:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-198.rdu2.redhat.com [10.10.120.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id 361AA2156712; Thu, 16 Aug 2018 20:04:59 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Michael D Kinney , Jiewen Yao , Eric Dong References: <20180816031422.15340-1-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 16 Aug 2018 22:04:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180816031422.15340-1-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 16 Aug 2018 20:05:01 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 16 Aug 2018 20:05:01 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Aug 2018 20:05:02 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Hao, On 08/16/18 05:14, Hao Wu wrote: > V2 changes: > A. Refine commit log message to clarify the purpose of the series > > B. Extract the RSB stuffing logic to INC files to avoid code duplication: > When compiling .NASM source files, the current build rule does not support > including files other than the .NASM file directory, this series will > duplicate the StuffRsb.inc file together with the .NASM files at this > moment. > > Please consider this approach as the first stage, I have filed a Bugzilla > for adding $(INC)-like support when compiling .NASM files: > https://bugzilla.tianocore.org/show_bug.cgi?id=1085 > > After the above support is added, the next step will be taken to remove > those duplicated StuffRsb.inc files and put it under a common include > directory like: > UefiCpuPkg/Include/ > > Cc: Jiewen Yao > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Michael D Kinney > > Hao Wu (2): > UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction > UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction this looks better, much appreciated. I've checked the reference from Jiewen, namely . Related to that, I have a number of questions / requests. The Intel publication linked above names two CVEs, CVE-2017-5753 and CVE-2017-5715. The patches are clearly relevant for CVE-2017-5715 (RSB stuffing before RSM). However, I'm unsure if the patches are also relevant for CVE-2017-5753 ("LFENCE after validation of untrusted data but before use"). The patches contain LFENCE instructions, but they don't seem to separate data validation from data use -- they are in the middle of the SpecTrap loops. What is their purpose? Are they meant to prevent speculation past the JMP instructions? (1) So, my first request is, please add the *exact* CVE number(s) to the subject lines of the patches. (Even if this makes the subjects a bit too long.) It is important to see the CVE numbers in a shortlog, such as "git log --oneline". (2) The URL of the Intel publication linked above is wrapped in both commit messages. Please make sure they aren't wrapped. It's OK if they end up being so long that we would normally not accept them in commit messages. They are URLs and should be easy to click, or copy&paste. (3) If we have (hidden) TianoCore BZs for these CVEs, they should be opened up to the public, and they should be referenced in the commit messages (in parallel to (1) -- that is, let's state which CVEs are addressed by the patches, and then name the matching TianoCore BZs as well). Other than that, the commit messages do a good job at explaining that these firmware patches protect the retpolines in the *OS*. The article says the same, but including those sentences in the commit messages is best. I'll proceed to reviewing and testing the patches. Thanks Laszlo