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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 17, 2018 4:05 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Yao, Jiewen; Dong, Eric
> 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-
> firmware-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?

There is a public document for retpoline at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/Retpoline-A-Branch-Target-Injection-Mitigation.pdf

Within section '4.4 Speculation Barriers', I find that:

"
The architectural specification for LFENCE defines that it does not
execute until all prior instructions have completed, and no later
instructions begin execution until LFENCE completes. This specification
limits the speculative execution that a processor implementation can
perform around the LFENCE, ...
"

I think, just as you mentioned, the lfence within the 'trap' is to limit
the speculative execution beyond JMP.

> 
> (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".

Yes. I will refine the subject of each commits to contain the CVE number.

> 
> (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.

Yes. I will modify the log message and keep the URL in one line.

> 
> (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).

It seems I cannot find a BZ for this, I will submit one and update the log
message to reference the BZ.

Best Regards,
Hao Wu

> 
> 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