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

[Laszlo Ersek <lersek@redhat.com>]

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?

(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