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

 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm     |  3 ++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 10 ++--
 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc      | 55 ++++++++++++++++++++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm      |  3 ++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm  |  8 ++-
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc       | 55 ++++++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm                |  3 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm                 |  3 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc                 | 55 ++++++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm                 |  3 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm                  |  3 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/StuffRsb.inc                  | 55 ++++++++++++++++++++
 12 files changed, 251 insertions(+), 5 deletions(-)
 create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc
 create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/X64/StuffRsb.inc

-- 
2.12.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/16/18 22:04, Laszlo Ersek wrote:

> (1) [...]
> (2) [...]
> (3) [...]

(4) Please reference
<https://bugzilla.tianocore.org/show_bug.cgi?id=1091> in the commit
messages as well; which is about the unification of the INC files.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Friday, August 17, 2018 5:09 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
> 
> On 08/16/18 22:04, Laszlo Ersek wrote:
> 
> > (1) [...]
> > (2) [...]
> > (3) [...]
> 
> (4) Please reference
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1091> in the commit
> messages as well; which is about the unification of the INC files.

Yes, I will mention this BZ dependency in the log message.

Best Regards,
Hao Wu

> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

> -----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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel