Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction

["Yao, Jiewen" <jiewen.yao@intel.com>]

Laszlo
Good question on CVE.

This patch only handles the Spectre variant 2 - CVE 2017-5715.

There will be separate patch for Spectre variant 1 - CVE 2017-5753. It is under way. :)

Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 17, 2018 4:05 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM
> instruction
> 
> 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 <jiewen.yao@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > 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
> <https://software.intel.com/security-software-guidance/insights/host-firmwar
> e-speculative-execution-side-channel-mitigation>.
> 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