[edk2] [Patch][edk2-platforms/devel-IntelAtomProcessorE3900] Setup Cleanup

[zwei4]

Remove TPM setup option. Add fixed PCD 
gPlatformModuleTokenSpaceGuid.PcdTpmControl to enable/disable TPM at build-time.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: David Wei 
CC: Mike Wu  
CC: Mang Guo 
CC: Steele Kelly 
---
 .../Common/Include/Guid/SetupVariable.h|  2 +-
 .../PeiPlatformConfigUpdateLib.c   | 36 +---
 .../PlatformPostMemPei/PlatformInit.c  | 15 +++--
 .../PlatformPostMemPei/PlatformPostMemPei.inf  |  3 +-
 .../PlatformSetupDxe/PlatformSetupDxe.c|  7 +--
 .../PlatformSettings/PlatformSetupDxe/Security.vfi | 25 +++--
 .../PlatformSetupDxe/SetupInfoRecords.c| 64 +-
 Platform/BroxtonPlatformPkg/PlatformPkg.dec|  4 ++
 8 files changed, 26 insertions(+), 130 deletions(-)

diff --git a/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h 
b/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h
index 19b948c0ea..2d81068778 100644
--- a/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h
+++ b/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h
@@ -480,7 +480,7 @@ typedef struct {
   UINT8 PanelScaling;
   UINT8 IgdLcdIGmchBlc;
   UINT8 SecEnable;
-  UINT8 TPM;
+  UINT8 TPMReserved;
   UINT8 TPMSupportedBanks;
   UINT8 TpmDetection;
   UINT8 PttSuppressCommandSend;  // For PTT Debug
diff --git 
a/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
 
b/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
index a003b278b4..b62db3ae8d 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
@@ -2,7 +2,7 @@
   Platform Configuration Update library implementation file.
   This library updates the setup data with platform overrides.
 
-  Copyright (c) 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
@@ -30,37 +30,6 @@
 #define SETUP_NFC_IPT  1
 #define SETUP_NFC  2
 
-EFI_STATUS
-TpmSetupPolicyInit (
-  IN SYSTEM_CONFIGURATION*SystemConfiguration
-  )
-{
-#if FTPM_SUPPORT
-  EFI_STATUS   Status;
-  BOOLEAN  PttEnabledState = FALSE;
-  EFI_HOB_GUID_TYPE*FdoEnabledGuidHob = NULL;
-
-  FdoEnabledGuidHob = GetFirstGuidHob ();
-
-  if (SystemConfiguration->TpmDetection == 0) {
-Status = PttHeciGetState ();
-if (EFI_ERROR (Status)) {
-  DEBUG ((DEBUG_ERROR, "Get PTT enabled state failed.\n"));
-}
-
-if (PttEnabledState && (FdoEnabledGuidHob == NULL)) {
-  SystemConfiguration->TPM = TPM_PTT;
-} else {
-  DEBUG ((EFI_D_INFO, "TpmPolicyInit-TPM and TpmDetection is disabled 
because of FDO \n\r"));
-  SystemConfiguration->TPM = TPM_DISABLE;
-}
-SystemConfiguration->TpmDetection = 1;
-  }
-
-#endif
-  return EFI_SUCCESS;
-}
-
 
 EFI_STATUS
 GetSecureNfcInfo (
@@ -189,9 +158,6 @@ UpdateSetupDataValues (
   Status = GetSecureNfcInfo (PreDefaultSetupData);
   ASSERT_EFI_ERROR (Status);
 
-  Status = TpmSetupPolicyInit (PreDefaultSetupData);
-  ASSERT_EFI_ERROR (Status);
-
   return Status;
 }
 
diff --git 
a/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPostMemPei/PlatformInit.c
 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPostMemPei/PlatformInit.c
index acaaebbfbb..412304b158 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPostMemPei/PlatformInit.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPostMemPei/PlatformInit.c
@@ -76,19 +76,22 @@ TpmPolicyInit (
   EFI_STATUS  Status;
   UINTN   Size;
   BOOLEAN IsPttEnabled = TRUE;
-  MBP_ME_FW_CAPS  Mbp_Me_Fw_Caps = { 0 };
+  MBP_ME_FW_CAPS  MbpMeFwCaps = { 0 };
+  UINT8   TpmControl;
 
   DEBUG ((EFI_D_ERROR, "TpmPolicyInit entry.\n"));
 
+  TpmControl = PcdGet8(PcdTpmControl);
+  
   //
   // Get ME FW Capability from MBP table to determine PTT State
   //
-  Status = HeciGetMeFwCapability (_Me_Fw_Caps);
+  Status = HeciGetMeFwCapability ();
   if (!EFI_ERROR (Status)) {
-IsPttEnabled = (BOOLEAN) Mbp_Me_Fw_Caps.CurrentFeatures.Fields.PTT;
+IsPttEnabled = (BOOLEAN) MbpMeFwCaps.CurrentFeatures.Fields.PTT;
   }
 
-  if ((IsPttEnabled) && (SystemConfiguration->TPM == TPM_PTT)) {
+  if ((IsPttEnabled) && (TpmControl == TPM_PTT)) {
 if (SystemConfiguration->PttSuppressCommandSend == 1) {
   Size = sizeof (gEfiTpmDeviceInstanceNoneGuid);
   PcdSetPtrS (PcdTpmInstanceGuid, , );
@@ -101,14 +104,14 @@ 

Re: [edk2] [PATCH v1] MinPlatformPkg/Test: [CVE-2017-5753] Fix bounds check bypass

[Wu, Hao A]

Please ignore this one.

Some more information need to be added in the subject line.
Another patch mail has been sent out to address this.

Best Regards,
Hao Wu


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Hao
> Wu
> Sent: Sunday, September 30, 2018 1:26 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Yao, Jiewen
> Subject: [edk2] [PATCH v1] MinPlatformPkg/Test: [CVE-2017-5753] Fix bounds
> check bypass
> 
> Speculative execution is used by processor to avoid having to wait for
> data to arrive from memory, or for previous operations to finish, the
> processor may speculate as to what will be executed.
> 
> If the speculation is incorrect, the speculatively executed instructions
> might leave hints such as which memory locations have been brought into
> cache. Malicious actors can use the bounds check bypass method (code
> gadgets with controlled external inputs) to infer data values that have
> been used in speculative operations to reveal secrets which should not
> otherwise be accessed.
> 
> This commit will focus on the SMI handler(s) registered within
> TestPointCheckLib & TestPointLib and insert AsmLfence API to mitigate the
> bounds check bypass issue.
> 
> A. For SMI handler TestPointSmmHandler() within TestPointCheckLib:
> 
> Under "case
> TEST_POINT_SMM_COMMUNICATION_FUNC_ID_UEFI_GCD_MAP_INFO:",
> 'CommBuffer' (controlled external inputs) is passed into function
> TestPointSmmReadyToBootSmmPageProtectionHandler().
> 
> Within function TestPointSmmReadyToBootSmmPageProtectionHandler(), the
> contents in 'CommBuffer' will be copied into 'CommData'. But if the size
> and sanity checks for the communication buffer is speculatively bypassed,
> '(UINTN)CommData + CommData->UefiMemoryMapOffset)' can potentially
> point
> to cross boundary area of 'CommData'. This pointer is then passed into
> function TestPointCheckSmmCommunicationBuffer() as 'UefiMemoryMap'.
> 
> Within function TestPointCheckSmmCommunicationBuffer(),
> 'MemoryMap->PhysicalStart' can be a potential cross boundary access. And
> its value can be inferred by function calls sequence:
> 
> TestPointCheckPageTable() via 'BaseAddress'
> GetPageTableEntry() via 'BaseAddress'. Then one can observe which part of
> the content within arrays 'L4PageTable', 'L3PageTable', 'L2PageTable' or
> 'L1PageTable', was brought into cache to possibly reveal the value.
> 
> B. For SMI handler SmmTestPointSmiHandler() within TestPointLib:
> 
> Under "case
> SMI_HANDLER_TEST_POINT_COMMAND_GET_DATA_BY_OFFSET:",
> 'CommBuffer' (controlled external inputs) is passed into function
> SmmTestPointSmiHandlerGetDataByOffset().
> 
> Within function SmmTestPointSmiHandlerGetDataByOffset(), the contents in
> 'CommBuffer' will be copied into 'SmiHandlerTestPointGetDataByOffset'. But
> if the size and sanity checks for the communication buffer is
> speculatively bypassed, 'SmiHandlerTestPointGetDataByOffset.DataSize' can
> be a potential cross boundary access.
> 
> Then in function SmiHandlerTestPointCopyData(), this value can be inferred
> by code:
>   CopyMem(
> DataBuffer,
> (UINT8 *)InputData + *DataOffset,
> (UINTN)*DataSize
> );
> One can observe which part of the content within 'DataBuffer' was brought
> into cache to possibly reveal the cross bounary access value.
> 
> Hence, this commit adds AsmLfence() calls after the boundary/range checks
> of the communication buffer to prevent the speculative execution.
> 
> A more detailed explanation of the purpose of commit is under the
> 'Bounds check bypass mitigation' section of the below link:
> https://software.intel.com/security-software-guidance/insights/host-firmware-
> speculative-execution-side-channel-mitigation
> 
> And the document at:
> https://software.intel.com/security-software-guidance/api-
> app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-
> vulnerabilities.pdf
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu 
> ---
> 
> Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPoint
> CheckLib.c | 7 +++
> 
> Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointComm
> unication.c | 8 +++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPoi
> ntCheckLib.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPoi
> ntCheckLib.c
> index b40469b278..dc40dae6d5 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPoi
> ntCheckLib.c
> +++
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPoi
> ntCheckLib.c
> @@ -14,6 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -374,6 +375,12 @@
> TestPointSmmReadyToBootSmmPageProtectionHandler (

[edk2] [PATCH v1][edk2-platforms/devel-MinPlatform] MinPlatformPkg/Test: [CVE-2017-5753] Fix bounds check bypass

[Hao Wu]

Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.

This commit will focus on the SMI handler(s) registered within
TestPointCheckLib & TestPointLib and insert AsmLfence API to mitigate the
bounds check bypass issue.

A. For SMI handler TestPointSmmHandler() within TestPointCheckLib:

Under "case TEST_POINT_SMM_COMMUNICATION_FUNC_ID_UEFI_GCD_MAP_INFO:",
'CommBuffer' (controlled external inputs) is passed into function
TestPointSmmReadyToBootSmmPageProtectionHandler().

Within function TestPointSmmReadyToBootSmmPageProtectionHandler(), the
contents in 'CommBuffer' will be copied into 'CommData'. But if the size
and sanity checks for the communication buffer is speculatively bypassed,
'(UINTN)CommData + CommData->UefiMemoryMapOffset)' can potentially point
to cross boundary area of 'CommData'. This pointer is then passed into
function TestPointCheckSmmCommunicationBuffer() as 'UefiMemoryMap'.

Within function TestPointCheckSmmCommunicationBuffer(),
'MemoryMap->PhysicalStart' can be a potential cross boundary access. And
its value can be inferred by function calls sequence:

TestPointCheckPageTable() via 'BaseAddress'
GetPageTableEntry() via 'BaseAddress'. Then one can observe which part of
the content within arrays 'L4PageTable', 'L3PageTable', 'L2PageTable' or
'L1PageTable', was brought into cache to possibly reveal the value.

B. For SMI handler SmmTestPointSmiHandler() within TestPointLib:

Under "case SMI_HANDLER_TEST_POINT_COMMAND_GET_DATA_BY_OFFSET:",
'CommBuffer' (controlled external inputs) is passed into function
SmmTestPointSmiHandlerGetDataByOffset().

Within function SmmTestPointSmiHandlerGetDataByOffset(), the contents in
'CommBuffer' will be copied into 'SmiHandlerTestPointGetDataByOffset'. But
if the size and sanity checks for the communication buffer is
speculatively bypassed, 'SmiHandlerTestPointGetDataByOffset.DataSize' can
be a potential cross boundary access.

Then in function SmiHandlerTestPointCopyData(), this value can be inferred
by code:
  CopyMem(
DataBuffer,
(UINT8 *)InputData + *DataOffset,
(UINTN)*DataSize
);
One can observe which part of the content within 'DataBuffer' was brought
into cache to possibly reveal the cross bounary access value.

Hence, this commit adds AsmLfence() calls after the boundary/range checks
of the communication buffer to prevent the speculative execution.

A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 
Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
 | 7 +++
 
Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointCommunication.c
 | 8 +++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git 
a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
 
b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
index b40469b278..dc40dae6d5 100644
--- 
a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
+++ 
b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
@@ -14,6 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -374,6 +375,12 @@ TestPointSmmReadyToBootSmmPageProtectionHandler (
   }
 
   if (CommData->UefiMemoryMapSize != 0) {
+//
+// The AsmLfence() call here is to ensure the previous range/content checks
+// for the CommBuffer (copied in to CommData) have been completed before
+// calling into TestPointCheckSmmCommunicationBuffer().
+//
+AsmLfence ();
 Result = TRUE;
 
 Status = TestPointCheckSmmCommunicationBuffer (
diff --git 
a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointCommunication.c
 
b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointCommunication.c
index cce0538832..b4757da046 100644
--- 

[edk2] [PATCH v1] MinPlatformPkg/Test: [CVE-2017-5753] Fix bounds check bypass

[Hao Wu]

Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.

This commit will focus on the SMI handler(s) registered within
TestPointCheckLib & TestPointLib and insert AsmLfence API to mitigate the
bounds check bypass issue.

A. For SMI handler TestPointSmmHandler() within TestPointCheckLib:

Under "case TEST_POINT_SMM_COMMUNICATION_FUNC_ID_UEFI_GCD_MAP_INFO:",
'CommBuffer' (controlled external inputs) is passed into function
TestPointSmmReadyToBootSmmPageProtectionHandler().

Within function TestPointSmmReadyToBootSmmPageProtectionHandler(), the
contents in 'CommBuffer' will be copied into 'CommData'. But if the size
and sanity checks for the communication buffer is speculatively bypassed,
'(UINTN)CommData + CommData->UefiMemoryMapOffset)' can potentially point
to cross boundary area of 'CommData'. This pointer is then passed into
function TestPointCheckSmmCommunicationBuffer() as 'UefiMemoryMap'.

Within function TestPointCheckSmmCommunicationBuffer(),
'MemoryMap->PhysicalStart' can be a potential cross boundary access. And
its value can be inferred by function calls sequence:

TestPointCheckPageTable() via 'BaseAddress'
GetPageTableEntry() via 'BaseAddress'. Then one can observe which part of
the content within arrays 'L4PageTable', 'L3PageTable', 'L2PageTable' or
'L1PageTable', was brought into cache to possibly reveal the value.

B. For SMI handler SmmTestPointSmiHandler() within TestPointLib:

Under "case SMI_HANDLER_TEST_POINT_COMMAND_GET_DATA_BY_OFFSET:",
'CommBuffer' (controlled external inputs) is passed into function
SmmTestPointSmiHandlerGetDataByOffset().

Within function SmmTestPointSmiHandlerGetDataByOffset(), the contents in
'CommBuffer' will be copied into 'SmiHandlerTestPointGetDataByOffset'. But
if the size and sanity checks for the communication buffer is
speculatively bypassed, 'SmiHandlerTestPointGetDataByOffset.DataSize' can
be a potential cross boundary access.

Then in function SmiHandlerTestPointCopyData(), this value can be inferred
by code:
  CopyMem(
DataBuffer,
(UINT8 *)InputData + *DataOffset,
(UINTN)*DataSize
);
One can observe which part of the content within 'DataBuffer' was brought
into cache to possibly reveal the cross bounary access value.

Hence, this commit adds AsmLfence() calls after the boundary/range checks
of the communication buffer to prevent the speculative execution.

A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 
Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
 | 7 +++
 
Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointCommunication.c
 | 8 +++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git 
a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
 
b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
index b40469b278..dc40dae6d5 100644
--- 
a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
+++ 
b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.c
@@ -14,6 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -374,6 +375,12 @@ TestPointSmmReadyToBootSmmPageProtectionHandler (
   }
 
   if (CommData->UefiMemoryMapSize != 0) {
+//
+// The AsmLfence() call here is to ensure the previous range/content checks
+// for the CommBuffer (copied in to CommData) have been completed before
+// calling into TestPointCheckSmmCommunicationBuffer().
+//
+AsmLfence ();
 Result = TRUE;
 
 Status = TestPointCheckSmmCommunicationBuffer (
diff --git 
a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointCommunication.c
 
b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointCommunication.c
index cce0538832..b4757da046 100644
--- 

Re: [edk2] [PATCH v3 0/5] [CVE-2017-5753] Bounds Check Bypass issue in SMI handlers

[Wu, Hao A]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Sunday, September 30, 2018 1:34 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Yao, Jiewen
> Subject: Re: [edk2] [PATCH v3 0/5] [CVE-2017-5753] Bounds Check Bypass issue
> in SMI handlers
> 
> On 09/29/18 08:57, Hao Wu wrote:
> > V3 changes:
> > A. Fix wrong file (should be LoadFenceSmm.c) gets listed in
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> >
> > B. Rename the newly introduced internal function from 'VariableLoadFence'
> >to 'MemoryLoadFence' within
> >MdeModulePkg/Universal/Variable/RuntimeDxe/
> >
> > C. Remove extra empty line before EOF for files:
> >MdePkg/Library/BaseLib/Ia32/Lfence.nasm
> >MdePkg/Library/BaseLib/X64/Lfence.nasm
> >
> > D. Remove dummy message within commit log messages
> >
> > Since A. has functional impact. Thus send V3 of the series.
> 
> series
> Acked-by: Laszlo Ersek 
> Regression-tested-by: Laszlo Ersek 

Thanks a lot for the comments and tests.
Series been pushed at 2ecd829972..5b02be4d9a.

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

Re: [edk2] [Patch] FDF spec: Support Structure PCD field assignment syntax in [Defines] section

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong


-Original Message-
From: Gao, Liming 
Sent: Sunday, September 30, 2018 10:08 AM
To: edk2-devel@lists.01.org
Cc: Zhu, Yonghong 
Subject: [Patch] FDF spec: Support Structure PCD field assignment syntax in 
[Defines] section

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
Cc: Yonghong Zhu 
---
 3_edk_ii_fdf_file_format/34_[defines]_section.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/3_edk_ii_fdf_file_format/34_[defines]_section.md 
b/3_edk_ii_fdf_file_format/34_[defines]_section.md
index 4c8d08f..1bc30d9 100644
--- a/3_edk_ii_fdf_file_format/34_[defines]_section.md
+++ b/3_edk_ii_fdf_file_format/34_[defines]_section.md
@@ -61,7 +61,7 @@ Conditional statements may be used anywhere within this 
section.
::= {} {} {}
  ::= 
 ::= {} {(0-9)+ "." (0-9)+}
-   ::=  "SET"[] 
+   ::=  "SET"  {} {}  [] 

   ::= {} {} {} {}
  {} {} {}  ```
--
2.10.0.windows.1

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

[edk2] [Patch] FDF spec: Support Structure PCD field assignment syntax in [Defines] section

[Liming Gao]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
Cc: Yonghong Zhu 
---
 3_edk_ii_fdf_file_format/34_[defines]_section.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/3_edk_ii_fdf_file_format/34_[defines]_section.md 
b/3_edk_ii_fdf_file_format/34_[defines]_section.md
index 4c8d08f..1bc30d9 100644
--- a/3_edk_ii_fdf_file_format/34_[defines]_section.md
+++ b/3_edk_ii_fdf_file_format/34_[defines]_section.md
@@ -61,7 +61,7 @@ Conditional statements may be used anywhere within this 
section.
::= {} {} {}
  ::= 
 ::= {} {(0-9)+ "." (0-9)+}
-   ::=  "SET"[] 
+   ::=  "SET"  {} {}  [] 

   ::= {} {} {} {}
  {} {} {}
 ```
-- 
2.10.0.windows.1

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

Re: [edk2] [Patch] BaseTools: refactor the error for PCD value is negative or exceed max

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong 
Zhu
Sent: Saturday, September 29, 2018 11:08 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: [edk2] [Patch] BaseTools: refactor the error for PCD value is negative 
or exceed max

From: zhijufan 

refactor the error handling for the PCD value that is negative or it exceed the 
max value.

Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Source/Python/AutoGen/GenC.py | 57 - 
 BaseTools/Source/Python/BPDG/GenVpd.py  | 44 -  
BaseTools/Source/Python/Common/Misc.py  |  4 +++
 3 files changed, 30 insertions(+), 75 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenC.py 
b/BaseTools/Source/Python/AutoGen/GenC.py
index f455f83..09626d0 100644
--- a/BaseTools/Source/Python/AutoGen/GenC.py
+++ b/BaseTools/Source/Python/AutoGen/GenC.py
@@ -1013,54 +1013,23 @@ def CreateModulePcdCode(Info, AutoGenC, AutoGenH, Pcd):
 ValueNumber = int (Value, 0)
 except:
 EdkLogger.error("build", AUTOGEN_ERROR,
 "PCD value is not valid dec or hex number for 
datum type [%s] of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, 
TokenCName),
 ExtraData="[%s]" % str(Info))
-if Pcd.DatumType == TAB_UINT64:
-if ValueNumber < 0:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"PCD can't be set to negative value for 
datum type [%s] of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, 
TokenCName),
-ExtraData="[%s]" % str(Info))
-elif ValueNumber >= 0x1:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"Too large PCD value for datum type [%s] 
of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, TokenCName),
-ExtraData="[%s]" % str(Info))
-if not Value.endswith('ULL'):
-Value += 'ULL'
-elif Pcd.DatumType == TAB_UINT32:
-if ValueNumber < 0:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"PCD can't be set to negative value for 
datum type [%s] of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, 
TokenCName),
-ExtraData="[%s]" % str(Info))
-elif ValueNumber >= 0x1:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"Too large PCD value for datum type [%s] 
of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, TokenCName),
-ExtraData="[%s]" % str(Info))
-if not Value.endswith('U'):
-Value += 'U'
-elif Pcd.DatumType == TAB_UINT16:
-if ValueNumber < 0:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"PCD can't be set to negative value for 
datum type [%s] of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, 
TokenCName),
-ExtraData="[%s]" % str(Info))
-elif ValueNumber >= 0x1:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"Too large PCD value for datum type [%s] 
of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, TokenCName),
-ExtraData="[%s]" % str(Info))
-if not Value.endswith('U'):
-Value += 'U'
-elif Pcd.DatumType == TAB_UINT8:
-if ValueNumber < 0:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"PCD can't be set to negative value for 
datum type [%s] of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, 
TokenCName),
-ExtraData="[%s]" % str(Info))
-elif ValueNumber >= 0x100:
-EdkLogger.error("build", AUTOGEN_ERROR,
-"Too large PCD value for datum type [%s] 
of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, TokenCName),
-ExtraData="[%s]" % str(Info))
-if not Value.endswith('U'):
-Value += 'U'
+if ValueNumber < 0:
+EdkLogger.error("build", AUTOGEN_ERROR,
+"PCD can't be set to negative value for datum 
type [%s] of PCD %s.%s" % (Pcd.DatumType, Pcd.TokenSpaceGuidCName, TokenCName),
+ExtraData="[%s]" % str(Info))
+elif ValueNumber > MAX_VAL_TYPE[Pcd.DatumType]:

Re: [edk2] [edk2-ModuleWriteGuide] Create repository for EDKII Module Write Guide document

[Gao, Liming]

Hi, all
  EDKII Module Write Guide gitboot contents have been published into 
https://github.com/tianocore-docs/edk2-ModuleWriteGuide.git.
  And, this doc is also added into 
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Draft-Specification.
 If you have interest, please help review. If you find any issue, please let me 
know. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gao, 
> Liming
> Sent: Tuesday, September 18, 2018 4:26 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Shaw, Kevin W 
> 
> Subject: [edk2] [edk2-ModuleWriteGuide] Create repository for EDKII Module 
> Write Guide document
> 
> Hi, all
>   We convert EDKII Module Write Guide
> (https://github.com/tianocore-docs/Docs/raw/master/User_Docs/EDK_II%20Module%20Writer_s%20Guide_0_7.pdf)
>  to gitbook.
>   We will push its contents to gitbook url 
> https://github.com/tianocore-docs/edk2-ModuleWriteGuide.git.
> 
> Thanks
> Liming
> ___
> 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

Re: [edk2] [PATCH] ShellPkg ShellLib.h: Fix wrong parameter name for ShellOpenFileByName

[Zeng, Star]

Ray,

Thanks.

I did not check the detail. But at least, one of definition and implementation 
needs to be updated. Maybe the description need to be enhanced also as you 
described.

ShellLib.h:

EFI_STATUS
EFIAPI
ShellOpenFileByDevicePath(
  IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
  OUT SHELL_FILE_HANDLE   *FileHandle,
  IN UINT64   OpenMode,
  IN UINT64   Attributes
  );

EFI_STATUS
EFIAPI
ShellOpenFileByName(
  IN CONST CHAR16   *FilePath,
  OUT SHELL_FILE_HANDLE *FileHandle,
  IN UINT64 OpenMode,
  IN UINT64 Attributes
  );


UefiShellLib.c:

EFI_STATUS
EFIAPI
ShellOpenFileByDevicePath(
  IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
  OUT SHELL_FILE_HANDLE   *FileHandle,
  IN UINT64   OpenMode,
  IN UINT64   Attributes
  )

EFI_STATUS
EFIAPI
ShellOpenFileByName(
  IN CONST CHAR16   *FileName,
  OUT SHELL_FILE_HANDLE *FileHandle,
  IN UINT64 OpenMode,
  IN UINT64 Attributes
  )

Star
-Original Message-
From: Ni, Ruiyu 
Sent: Sunday, September 30, 2018 9:12 AM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Carsey, Jaben 
Subject: RE: [PATCH] ShellPkg ShellLib.h: Fix wrong parameter name for 
ShellOpenFileByName

Star,
Per my understanding, FilePath means the full path pointing to the file, 
FileName means the very last part of the full path.
E.g.: "fs0:\a\b\c\d.txt" is a FilePath, "d.txt" is a FileName.

So I think only specifying FileName is not enough to identify a file.
The parameter name should be fine.

> -Original Message-
> From: Zeng, Star
> Sent: Saturday, September 29, 2018 5:43 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Ni, Ruiyu ; 
> Carsey, Jaben 
> Subject: [PATCH] ShellPkg ShellLib.h: Fix wrong parameter name for 
> ShellOpenFileByName
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1221
> 
> The parameter name FilePath should be FileName.
> 
> I am trying to write an application for my own use and want to use 
> this interface, but confused by the parameter name.
> 
> Interesting, the implementation in UefiShellLib.c is correct.
> 
> Cc: Ruiyu Ni 
> Cc: Jaben Carsey 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  ShellPkg/Include/Library/ShellLib.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Include/Library/ShellLib.h
> b/ShellPkg/Include/Library/ShellLib.h
> index 92fddc50f5dd..d6a5319285dd 100644
> --- a/ShellPkg/Include/Library/ShellLib.h
> +++ b/ShellPkg/Include/Library/ShellLib.h
> @@ -126,7 +126,7 @@ ShellOpenFileByDevicePath(
>otherwise, the Filehandle is NULL. Attributes is valid only for
>EFI_FILE_MODE_CREATE.
> 
> -  @param[in] FilePath   The pointer to file name.
> +  @param[in] FileName   The pointer to file name.
>@param[out] FileHandleThe pointer to the file handle.
>@param[in] OpenMode   The mode to open the file with.
>@param[in] Attributes The file's file attributes.
> @@ -151,7 +151,7 @@ ShellOpenFileByDevicePath(  EFI_STATUS  EFIAPI 
> ShellOpenFileByName(
> -  IN CONST CHAR16   *FilePath,
> +  IN CONST CHAR16   *FileName,
>OUT SHELL_FILE_HANDLE *FileHandle,
>IN UINT64 OpenMode,
>IN UINT64 Attributes
> --
> 2.7.0.windows.1

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

Re: [edk2] [PATCH] ShellPkg ShellLib.h: Fix wrong parameter name for ShellOpenFileByName

[Ni, Ruiyu]

Star,
Per my understanding, FilePath means the full path pointing to the file,
FileName means the very last part of the full path.
E.g.: "fs0:\a\b\c\d.txt" is a FilePath, "d.txt" is a FileName.

So I think only specifying FileName is not enough to identify a file.
The parameter name should be fine.

> -Original Message-
> From: Zeng, Star
> Sent: Saturday, September 29, 2018 5:43 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Ni, Ruiyu ; Carsey,
> Jaben 
> Subject: [PATCH] ShellPkg ShellLib.h: Fix wrong parameter name for
> ShellOpenFileByName
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1221
> 
> The parameter name FilePath should be FileName.
> 
> I am trying to write an application for my own use and want to use this 
> interface,
> but confused by the parameter name.
> 
> Interesting, the implementation in UefiShellLib.c is correct.
> 
> Cc: Ruiyu Ni 
> Cc: Jaben Carsey 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  ShellPkg/Include/Library/ShellLib.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Include/Library/ShellLib.h
> b/ShellPkg/Include/Library/ShellLib.h
> index 92fddc50f5dd..d6a5319285dd 100644
> --- a/ShellPkg/Include/Library/ShellLib.h
> +++ b/ShellPkg/Include/Library/ShellLib.h
> @@ -126,7 +126,7 @@ ShellOpenFileByDevicePath(
>otherwise, the Filehandle is NULL. Attributes is valid only for
>EFI_FILE_MODE_CREATE.
> 
> -  @param[in] FilePath   The pointer to file name.
> +  @param[in] FileName   The pointer to file name.
>@param[out] FileHandleThe pointer to the file handle.
>@param[in] OpenMode   The mode to open the file with.
>@param[in] Attributes The file's file attributes.
> @@ -151,7 +151,7 @@ ShellOpenFileByDevicePath(  EFI_STATUS  EFIAPI
> ShellOpenFileByName(
> -  IN CONST CHAR16   *FilePath,
> +  IN CONST CHAR16   *FileName,
>OUT SHELL_FILE_HANDLE *FileHandle,
>IN UINT64 OpenMode,
>IN UINT64 Attributes
> --
> 2.7.0.windows.1

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

Re: [edk2] [Patch] UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h: Change to DOS format file.

[Bi, Dandan]

Reviewed-by: Dandan Bi 


Thanks,
Dandan
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Saturday, September 29, 2018 9:38 AM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan ; Laszlo Ersek 
> Subject: [edk2] [Patch]
> UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h: Change to DOS
> format file.
> 
> Follow EDKII coding style, change file format to dos style.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1213
> 
> Cc: Dandan Bi 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h | 544
> +++---
>  1 file changed, 272 insertions(+), 272 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h
> b/UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h
> index d050464b7f..64f3e14db3 100644
> --- a/UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h
> +++ b/UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h
> @@ -1,272 +1,272 @@
> -/** @file
> -  MSR Defintions for Intel Atom processors based on the Goldmont Plus
> microarchitecture.
> -
> -  Provides defines for Machine Specific Registers(MSR) indexes. Data
> structures
> -  are provided for MSRs that contain one or more bit fields.  If the MSR
> value
> -  returned is a single 32-bit or 64-bit value, then a data structure is not
> -  provided for that MSR.
> -
> -  Copyright (c) 2018, Intel Corporation. All rights reserved.
> -  This program and the accompanying materials
> -  are licensed and made available under the terms and conditions of the
> BSD License
> -  which accompanies this distribution.  The full text of the license may be
> found at
> -  http://opensource.org/licenses/bsd-license.php
> -
> -  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> -  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> -
> -  @par Specification Reference:
> -  Intel(R) 64 and IA-32 Architectures Software Developer's Manual, Volume
> 4,
> -  May 2018, Volume 4: Model-Specific-Registers (MSR)
> -
> -**/
> -
> -#ifndef __GOLDMONT_PLUS_MSR_H__
> -#define __GOLDMONT_PLUS_MSR_H__
> -
> -#include 
> -
> -/**
> -  Is Intel Atom processors based on the Goldmont plus microarchitecture?
> -
> -  @param   DisplayFamily  Display Family ID
> -  @param   DisplayModel   Display Model ID
> -
> -  @retval  TRUE   Yes, it is.
> -  @retval  FALSE  No, it isn't.
> -**/
> -#define IS_GOLDMONT_PLUS_PROCESSOR(DisplayFamily, DisplayModel) \
> -  (DisplayFamily == 0x06 && \
> -   (\
> -DisplayModel == 0x7A\
> -)   \
> -   )
> -
> -/**
> -  Core. (R/W) See Table 2-2. See Section 18.6.2.4, "Processor Event Based
> -  Sampling (PEBS).".
> -
> -  @param  ECX  MSR_GOLDMONT_PLUS_PEBS_ENABLE (0x03F1)
> -  @param  EAX  Lower 32-bits of MSR value.
> -   Described by the type
> MSR_GOLDMONT_PLUS_PEBS_ENABLE_REGISTER.
> -  @param  EDX  Upper 32-bits of MSR value.
> -   Described by the type
> MSR_GOLDMONT_PLUS_PEBS_ENABLE_REGISTER.
> -
> -  Example usage
> -  @code
> -  MSR_GOLDMONT_PLUS_PEBS_ENABLE_REGISTER  Msr;
> -
> -  Msr.Uint64 = AsmReadMsr64 (MSR_GOLDMONT_PLUS_PEBS_ENABLE);
> -  AsmWriteMsr64 (MSR_GOLDMONT_PLUS_PEBS_ENABLE, Msr.Uint64);
> -  @endcode
> -**/
> -#define MSR_GOLDMONT_PLUS_PEBS_ENABLE0x03F1
> -
> -/**
> -  MSR information returned for MSR index
> #MSR_GOLDMONT_PLUS_PEBS_ENABLE -**/ -typedef union {
> -  ///
> -  /// Individual bit fields
> -  ///
> -  struct {
> -///
> -/// [Bit 0] Enable PEBS trigger and recording for the programmed event
> -/// (precise or otherwise) on IA32_PMC0.
> -///
> -UINT32  Fix_Me_1:1;
> -///
> -/// [Bit 1] Enable PEBS trigger and recording for the programmed event
> -/// (precise or otherwise) on IA32_PMC1.
> -///
> -UINT32  Fix_Me_2:1;
> -///
> -/// [Bit 2] Enable PEBS trigger and recording for the programmed event
> -/// (precise or otherwise) on IA32_PMC2.
> -///
> -UINT32  Fix_Me_3:1;
> -///
> -/// [Bit 3] Enable PEBS trigger and recording for the programmed event
> -/// (precise or otherwise) on IA32_PMC3.
> -///
> -UINT32  Fix_Me_4:1;
> -UINT32  Reserved1:28;
> -///
> -/// [Bit 32] Enable PEBS trigger and recording for IA32_FIXED_CTR0.
> -///
> -UINT32  Fix_Me_5:1;
> -///
> -/// [Bit 33] Enable PEBS trigger and recording for IA32_FIXED_CTR1.
> -///
> -UINT32  Fix_Me_6:1;
> -///
> -/// [Bit 34] Enable PEBS trigger and recording for IA32_FIXED_CTR2.
> -///
> -UINT32  Fix_Me_7:1;
> -UINT32  Reserved2:29;
> -  } Bits;
> -  ///
> -  /// All bit fields as a 64-bit value
> -  ///
> -  UINT64  Uint64;
> -} MSR_GOLDMONT_PLUS_PEBS_ENABLE_REGISTER;
> -
> -
> -/**
> -  Core. Last Branch Record N From IP (R/W) One of the three MSRs that
> make up
> -  

Re: [edk2] [PATCH 0/5] MdePkg/BaseSynchronizationLib GCC: fixes, cleanups

[Laszlo Ersek]

On 09/30/18 00:23, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: inline_asm_rw_ops_1208
> 
> This series mainly fixes the operand constraints (missing input-output
> qualifications) in "BaseSynchronizationLib/*/GccInline.c".
> 
> (It would be better to remove these files altogether in favor of the
> already existing NASM implementation, but due to
> , we can't generally
> do that in libraries yet.)
> 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (5):
>   MdePkg/BaseSynchronizationLib GCC: fix whitespace and comments
>   MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange16()
>   MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange32()
>   MdePkg/BaseSynchronizationLib GCC: fix X64
> InternalSyncCompareExchange64()
>   MdePkg/BaseSynchronizationLib GCC: simplify IA32
> InternalSyncCompareExchange64()
> 
>  MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 42 +++--
>  MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c  | 47 
> +++-
>  2 files changed, 34 insertions(+), 55 deletions(-)
> 

Forgot to mention the testing.

Ran my usual OVMF tests described at

on multi-VCPU guests. I tested the series against all my longterm guests
(including 32-bit and 64-bit Fedora, and 64-bit Windows 7 / Server 2008
r2, 8.1 / Server 2012 r2, 10 / Server 2016). Also covered an SMM-less
build on i440fx, with 64-bit Fedora.

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

[edk2] [PATCH 5/5] MdePkg/BaseSynchronizationLib GCC: simplify IA32 InternalSyncCompareExchange64()

[Laszlo Ersek]

The IA32 variant of InternalSyncCompareExchange64() is correct, but we can
simplify it. We don't need to load the lower 32 bits of ExchangeValue into
EBX in two steps (first into a general register, then into EBX); we can
ask GCC to populate EBX like that itself.

Cc: Liming Gao 
Cc: Michael D Kinney 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1208
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
index 44188e265af2..af39bdeb516c 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
@@ -193,14 +193,11 @@ InternalSyncCompareExchange64 (
   )
 {
   __asm__ __volatile__ (
-"push%%ebx  \n\t"
-"movl%2,%%ebx   \n\t"
 "lock   \n\t"
 "cmpxchg8b   (%1)   \n\t"
-"pop %%ebx  \n\t"
 : "+A"  (CompareValue)// %0
 : "S"   (Value),  // %1
-  "r"   ((UINT32) ExchangeValue), // %2
+  "b"   ((UINT32) ExchangeValue), // %2
   "c"   ((UINT32) (ExchangeValue >> 32))  // %3
 : "memory",
   "cc"
-- 
2.14.1.3.gb7cf6e02401b

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

[edk2] [PATCH 3/5] MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange32()

[Laszlo Ersek]

(This patch is identical to the last one, except for the
InternalSyncCompareExchange16() -> InternalSyncCompareExchange32() and
"cmpxchgw" -> "cmpxchgl" replacements.)

The CMPXCHG instruction has the following operands:
- AX (implicit, CompareValue):input and output
- destination operand (*Value):   input and output
- source operand (ExchangeValue): input

The IA32 version of InternalSyncCompareExchange32() correctly marks
CompareValue as input/output, but it marks (*Value) only as input.

The X64 version of InternalSyncCompareExchange32() attempts to mark both
CompareValue and (*Value) as input/output, but it doesn't use the
appropriate constraints for either operand.

Fix these issues. Furthermore, prefer the short "+" constraint for I/O
operands over the  constraint that can be applied
to the input instances of I/O operands.

Cc: Liming Gao 
Cc: Michael D Kinney 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1208
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c |  9 -
 MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c  | 10 --
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
index bd98a5aebfe7..44188e265af2 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
@@ -155,11 +155,10 @@ InternalSyncCompareExchange32 (
 {
   __asm__ __volatile__ (
 "lock \n\t"
-"cmpxchgl%1, %2   \n\t"
-: "=a" (CompareValue)   // %0
-: "q"  (ExchangeValue), // %1
-  "m"  (*Value),// %2
-  "0"  (CompareValue)   // %3
+"cmpxchgl%2, %1   \n\t"
+: "+a" (CompareValue),  // %0
+  "+m" (*Value) // %1
+: "q"  (ExchangeValue)  // %2
 : "memory",
   "cc"
 );
diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
index 4338fb8e65e8..a85cf0265c8b 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
@@ -155,12 +155,10 @@ InternalSyncCompareExchange32 (
 {
   __asm__ __volatile__ (
 "lock \n\t"
-"cmpxchgl%3, %1   \n\t"
-: "=a" (CompareValue),  // %0
-  "=m" (*Value) // %1
-: "a"  (CompareValue),  // %2
-  "r"  (ExchangeValue), // %3
-  "m"  (*Value) // %4
+"cmpxchgl%2, %1   \n\t"
+: "+a" (CompareValue),  // %0
+  "+m" (*Value) // %1
+: "r"  (ExchangeValue)  // %2
 : "memory",
   "cc"
 );
-- 
2.14.1.3.gb7cf6e02401b


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

[edk2] [PATCH 2/5] MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange16()

[Laszlo Ersek]

The CMPXCHG instruction has the following operands:
- AX (implicit, CompareValue):input and output
- destination operand (*Value):   input and output
- source operand (ExchangeValue): input

The IA32 version of InternalSyncCompareExchange16() correctly marks
CompareValue as input/output, but it marks (*Value) only as input.

The X64 version of InternalSyncCompareExchange16() attempts to mark both
CompareValue and (*Value) as input/output, but it doesn't use the
appropriate constraints for either operand.

Fix these issues. Furthermore, prefer the short "+" constraint for I/O
operands over the  constraint that can be applied
to the input instances of I/O operands.

Cc: Liming Gao 
Cc: Michael D Kinney 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1208
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c |  9 -
 MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c  | 10 --
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
index 1976720ac636..bd98a5aebfe7 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
@@ -115,11 +115,10 @@ InternalSyncCompareExchange16 (
 {
   __asm__ __volatile__ (
 "lock \n\t"
-"cmpxchgw%1, %2   \n\t"
-: "=a" (CompareValue)   // %0
-: "q"  (ExchangeValue), // %1
-  "m"  (*Value),// %2
-  "0"  (CompareValue)   // %3
+"cmpxchgw%2, %1   \n\t"
+: "+a" (CompareValue),  // %0
+  "+m" (*Value) // %1
+: "q"  (ExchangeValue)  // %2
 : "memory",
   "cc"
 );
diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
index 0212798d7a27..4338fb8e65e8 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
@@ -115,12 +115,10 @@ InternalSyncCompareExchange16 (
 {
   __asm__ __volatile__ (
 "lock \n\t"
-"cmpxchgw%3, %1   \n\t"
-: "=a" (CompareValue),  // %0
-  "=m" (*Value) // %1
-: "a"  (CompareValue),  // %2
-  "r"  (ExchangeValue), // %3
-  "m"  (*Value) // %4
+"cmpxchgw%2, %1   \n\t"
+: "+a" (CompareValue),  // %0
+  "+m" (*Value) // %1
+: "r"  (ExchangeValue)  // %2
 : "memory",
   "cc"
 );
-- 
2.14.1.3.gb7cf6e02401b


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

[edk2] [PATCH 4/5] MdePkg/BaseSynchronizationLib GCC: fix X64 InternalSyncCompareExchange64()

[Laszlo Ersek]

(This patch is identical to the X64 half of the last one, except for the
InternalSyncCompareExchange32() -> InternalSyncCompareExchange64() and
"cmpxchgl" -> "cmpxchgq" replacements.)

The CMPXCHG instruction has the following operands:
- AX (implicit, CompareValue):input and output
- destination operand (*Value):   input and output
- source operand (ExchangeValue): input

The X64 version of InternalSyncCompareExchange64() attempts to mark both
CompareValue and (*Value) as input/output, but it doesn't use the
appropriate constraints for either operand.

Fix these issues. Furthermore, prefer the short "+" constraint for I/O
operands over the  constraint that can be applied
to the input instances of I/O operands.

Cc: Liming Gao 
Cc: Michael D Kinney 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1208
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
index a85cf0265c8b..edb904c00704 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
@@ -194,12 +194,10 @@ InternalSyncCompareExchange64 (
 {
   __asm__ __volatile__ (
 "lock \n\t"
-"cmpxchgq%3, %1   \n\t"
-: "=a" (CompareValue),  // %0
-  "=m" (*Value) // %1
-: "a"  (CompareValue),  // %2
-  "r"  (ExchangeValue), // %3
-  "m"  (*Value) // %4
+"cmpxchgq%2, %1   \n\t"
+: "+a" (CompareValue),  // %0
+  "+m" (*Value) // %1
+: "r"  (ExchangeValue)  // %2
 : "memory",
   "cc"
 );
-- 
2.14.1.3.gb7cf6e02401b


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

[edk2] [PATCH 1/5] MdePkg/BaseSynchronizationLib GCC: fix whitespace and comments

[Laszlo Ersek]

The "GccInline.c" files have some inconsistent whitespace, and missing (or
incorrect) operand comments. Fix and unify them.

This patch doesn't change behavior.

Cc: Liming Gao 
Cc: Michael D Kinney 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1208
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 35 ++---
 MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c  | 53 
+---
 2 files changed, 39 insertions(+), 49 deletions(-)

diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
index fa2be7f4b35c..1976720ac636 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
@@ -39,7 +39,7 @@ InternalSyncIncrement (
 "movl$1, %%eax  \n\t"
 "lock   \n\t"
 "xadd%%eax, %1  \n\t"
-"inc %%eax  "
+"inc %%eax  \n\t"
 : "=a" (Result),  // %0
   "+m" (*Value)   // %1
 : // no inputs that aren't also outputs
@@ -48,7 +48,6 @@ InternalSyncIncrement (
 );
 
   return Result;
-
 }
 
 
@@ -76,10 +75,10 @@ InternalSyncDecrement (
 "movl$-1, %%eax  \n\t"
 "lock\n\t"
 "xadd%%eax, %1   \n\t"
-"dec %%eax  "
-: "=a" (Result),  // %0
-  "+m" (*Value)   // %1
-: // no inputs that aren't also outputs
+"dec %%eax   \n\t"
+: "=a" (Result),   // %0
+  "+m" (*Value)// %1
+:  // no inputs that aren't also outputs
 : "memory",
   "cc"
 );
@@ -87,6 +86,7 @@ InternalSyncDecrement (
   return Result;
 }
 
+
 /**
   Performs an atomic compare exchange operation on a 16-bit unsigned integer.
 
@@ -113,15 +113,13 @@ InternalSyncCompareExchange16 (
   IN  UINT16ExchangeValue
   )
 {
-
   __asm__ __volatile__ (
-" \n\t"
 "lock \n\t"
 "cmpxchgw%1, %2   \n\t"
-: "=a" (CompareValue)
-: "q"  (ExchangeValue),
-  "m"  (*Value),
-  "0"  (CompareValue)
+: "=a" (CompareValue)   // %0
+: "q"  (ExchangeValue), // %1
+  "m"  (*Value),// %2
+  "0"  (CompareValue)   // %3
 : "memory",
   "cc"
 );
@@ -129,6 +127,7 @@ InternalSyncCompareExchange16 (
   return CompareValue;
 }
 
+
 /**
   Performs an atomic compare exchange operation on a 32-bit unsigned integer.
 
@@ -155,15 +154,13 @@ InternalSyncCompareExchange32 (
   IN  UINT32ExchangeValue
   )
 {
-
   __asm__ __volatile__ (
-" \n\t"
 "lock \n\t"
 "cmpxchgl%1, %2   \n\t"
-: "=a" (CompareValue) // %0
-: "q"  (ExchangeValue),   // %1
-  "m"  (*Value),  // %2
-  "0"  (CompareValue) // %4
+: "=a" (CompareValue)   // %0
+: "q"  (ExchangeValue), // %1
+  "m"  (*Value),// %2
+  "0"  (CompareValue)   // %3
 : "memory",
   "cc"
 );
@@ -171,6 +168,7 @@ InternalSyncCompareExchange32 (
   return CompareValue;
 }
 
+
 /**
   Performs an atomic compare exchange operation on a 64-bit unsigned integer.
 
@@ -197,7 +195,6 @@ InternalSyncCompareExchange64 (
   )
 {
   __asm__ __volatile__ (
-"   \n\t"
 "push%%ebx  \n\t"
 "movl%2,%%ebx   \n\t"
 "lock   \n\t"
diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c 
b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
index ab7efe23c4db..0212798d7a27 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
@@ -39,7 +39,7 @@ InternalSyncIncrement (
 "movl$1, %%eax  \n\t"
 "lock   \n\t"
 "xadd%%eax, %1  \n\t"
-"inc %%eax  "
+"inc %%eax  \n\t"
 : "=a" (Result),  // %0
   "+m" (*Value)   // %1
 : // no inputs that aren't also outputs
@@ -75,10 +75,10 @@ InternalSyncDecrement (
 "movl$-1, %%eax  \n\t"
 "lock\n\t"
 "xadd%%eax, %1   \n\t"
-"dec %%eax  "
-: "=a" (Result),  // %0
-  "+m" (*Value)   // %1
-: // no inputs that aren't also outputs
+"dec %%eax   \n\t"
+: "=a" (Result),   // %0
+  "+m" (*Value)// %1
+:  // no inputs that aren't also outputs
 : "memory",
   "cc"
 );
@@ -113,16 +113,14 @@ InternalSyncCompareExchange16 (
   IN  UINT16ExchangeValue
   )
 {
-
-
   __asm__ __volatile__ (
 "lock \n\t"
-"cmpxchgw%3, %1 

[edk2] [PATCH 0/5] MdePkg/BaseSynchronizationLib GCC: fixes, cleanups

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: inline_asm_rw_ops_1208

This series mainly fixes the operand constraints (missing input-output
qualifications) in "BaseSynchronizationLib/*/GccInline.c".

(It would be better to remove these files altogether in favor of the
already existing NASM implementation, but due to
, we can't generally
do that in libraries yet.)

Cc: Liming Gao 
Cc: Michael D Kinney 

Thanks,
Laszlo

Laszlo Ersek (5):
  MdePkg/BaseSynchronizationLib GCC: fix whitespace and comments
  MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange16()
  MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange32()
  MdePkg/BaseSynchronizationLib GCC: fix X64
InternalSyncCompareExchange64()
  MdePkg/BaseSynchronizationLib GCC: simplify IA32
InternalSyncCompareExchange64()

 MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 42 +++--
 MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c  | 47 
+++-
 2 files changed, 34 insertions(+), 55 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

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

Re: [edk2] [PATCH v3 0/5] [CVE-2017-5753] Bounds Check Bypass issue in SMI handlers

[Laszlo Ersek]

On 09/29/18 08:57, Hao Wu wrote:
> V3 changes:
> A. Fix wrong file (should be LoadFenceSmm.c) gets listed in
>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> 
> B. Rename the newly introduced internal function from 'VariableLoadFence'
>to 'MemoryLoadFence' within
>MdeModulePkg/Universal/Variable/RuntimeDxe/
> 
> C. Remove extra empty line before EOF for files:
>MdePkg/Library/BaseLib/Ia32/Lfence.nasm
>MdePkg/Library/BaseLib/X64/Lfence.nasm
> 
> D. Remove dummy message within commit log messages
> 
> Since A. has functional impact. Thus send V3 of the series.

series
Acked-by: Laszlo Ersek 
Regression-tested-by: Laszlo Ersek 

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

Re: [edk2] [Patch] UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h: Change to DOS format file.

[Laszlo Ersek]

On 09/29/18 03:37, Eric Dong wrote:
> Follow EDKII coding style, change file format to dos style.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1213
> 
> Cc: Dandan Bi 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h | 544 
> +++---
>  1 file changed, 272 insertions(+), 272 deletions(-)

Acked-by: Laszlo Ersek 

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

[edk2] [PATCH] ShellPkg ShellLib.h: Fix wrong parameter name for ShellOpenFileByName

[Star Zeng]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1221

The parameter name FilePath should be FileName.

I am trying to write an application for my own use and want to use
this interface, but confused by the parameter name.

Interesting, the implementation in UefiShellLib.c is correct.

Cc: Ruiyu Ni 
Cc: Jaben Carsey 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 ShellPkg/Include/Library/ShellLib.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Include/Library/ShellLib.h 
b/ShellPkg/Include/Library/ShellLib.h
index 92fddc50f5dd..d6a5319285dd 100644
--- a/ShellPkg/Include/Library/ShellLib.h
+++ b/ShellPkg/Include/Library/ShellLib.h
@@ -126,7 +126,7 @@ ShellOpenFileByDevicePath(
   otherwise, the Filehandle is NULL. Attributes is valid only for
   EFI_FILE_MODE_CREATE.
 
-  @param[in] FilePath   The pointer to file name.
+  @param[in] FileName   The pointer to file name.
   @param[out] FileHandleThe pointer to the file handle.
   @param[in] OpenMode   The mode to open the file with.
   @param[in] Attributes The file's file attributes.
@@ -151,7 +151,7 @@ ShellOpenFileByDevicePath(
 EFI_STATUS
 EFIAPI
 ShellOpenFileByName(
-  IN CONST CHAR16   *FilePath,
+  IN CONST CHAR16   *FileName,
   OUT SHELL_FILE_HANDLE *FileHandle,
   IN UINT64 OpenMode,
   IN UINT64 Attributes
-- 
2.7.0.windows.1

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

[edk2] [Patch] BaseTools: do basic check in FvImage with header size and signature

[Yonghong Zhu]

From: zhijufan 

Add some basic check in FvImage with header size and signature.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=1181
Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Source/Python/GenFds/Fv.py | 44 
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/BaseTools/Source/Python/GenFds/Fv.py 
b/BaseTools/Source/Python/GenFds/Fv.py
index 0d005eb..510f283 100644
--- a/BaseTools/Source/Python/GenFds/Fv.py
+++ b/BaseTools/Source/Python/GenFds/Fv.py
@@ -193,36 +193,40 @@ class FV (FvClassObject):
 )
 
 #
 # Write the Fv contents to Buffer
 #
-if os.path.isfile(FvOutputFile):
+if os.path.isfile(FvOutputFile) and os.path.getsize(FvOutputFile) 
>= 0x48:
 FvFileObj = open(FvOutputFile, 'rb')
-GenFdsGlobalVariable.VerboseLogger("\nGenerate %s FV 
Successfully" % self.UiFvName)
-GenFdsGlobalVariable.SharpCounter = 0
-
-Buffer.write(FvFileObj.read())
-FvFileObj.seek(0)
 # PI FvHeader is 0x48 byte
 FvHeaderBuffer = FvFileObj.read(0x48)
-# FV alignment position.
-FvAlignmentValue = 1 << (ord(FvHeaderBuffer[0x2E]) & 0x1F)
-if FvAlignmentValue >= 0x400:
-if FvAlignmentValue >= 0x10:
-if FvAlignmentValue >= 0x100:
-#The max alignment supported by FFS is 16M.
-self.FvAlignment = "16M"
+Signature = FvHeaderBuffer[0x28:0x32]
+if Signature and Signature.startswith('_FVH'):
+GenFdsGlobalVariable.VerboseLogger("\nGenerate %s FV 
Successfully" % self.UiFvName)
+GenFdsGlobalVariable.SharpCounter = 0
+
+Buffer.write(FvFileObj.read())
+FvFileObj.seek(0)
+# FV alignment position.
+FvAlignmentValue = 1 << (ord(FvHeaderBuffer[0x2E]) & 0x1F)
+if FvAlignmentValue >= 0x400:
+if FvAlignmentValue >= 0x10:
+if FvAlignmentValue >= 0x100:
+#The max alignment supported by FFS is 16M.
+self.FvAlignment = "16M"
+else:
+self.FvAlignment = str(FvAlignmentValue / 
0x10) + "M"
 else:
-self.FvAlignment = str(FvAlignmentValue / 
0x10) + "M"
+self.FvAlignment = str(FvAlignmentValue / 0x400) + 
"K"
 else:
-self.FvAlignment = str(FvAlignmentValue / 0x400) + "K"
+# FvAlignmentValue is less than 1K
+self.FvAlignment = str (FvAlignmentValue)
+FvFileObj.close()
+GenFdsGlobalVariable.ImageBinDict[self.UiFvName.upper() + 
'fv'] = FvOutputFile
+GenFdsGlobalVariable.LargeFileInFvFlags.pop()
 else:
-# FvAlignmentValue is less than 1K
-self.FvAlignment = str (FvAlignmentValue)
-FvFileObj.close()
-GenFdsGlobalVariable.ImageBinDict[self.UiFvName.upper() + 
'fv'] = FvOutputFile
-GenFdsGlobalVariable.LargeFileInFvFlags.pop()
+GenFdsGlobalVariable.ErrorLogger("Invalid FV file %s." % 
self.UiFvName)
 else:
 GenFdsGlobalVariable.ErrorLogger("Failed to generate %s FV 
file." %self.UiFvName)
 return FvOutputFile
 
 ## _GetBlockSize()
-- 
2.6.1.windows.1

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

[edk2] [Patch][edk2-platforms/devel-IntelAtomProcessorE3900] Cleanup Setup Option

[zwei4]

Remove SecureBoot setup option.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: David Wei 
CC: Mike Wu  
CC: Mang Guo 
CC: Steele Kelly 
---
 .../Smbios/SmBiosMiscDxe/MiscOemType0x94Function.c | 11 +-
 .../Smbios/SmBiosMiscDxe/SmBiosMiscDxe.inf |  1 +
 .../Common/Include/Guid/SetupVariable.h|  2 +-
 .../PeiPlatformConfigUpdateLib.c   |  3 +-
 .../PlatformSettings/PlatformSetupDxe/Boot.vfi |  7 
 .../PlatformSetupDxe/PlatformSetupDxe.c|  1 -
 .../PlatformSetupDxe/SetupInfoRecords.c| 45 --
 7 files changed, 13 insertions(+), 57 deletions(-)

diff --git 
a/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/MiscOemType0x94Function.c
 
b/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/MiscOemType0x94Function.c
index b399a5f81b..e96e3fb5a0 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/MiscOemType0x94Function.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/MiscOemType0x94Function.c
@@ -548,6 +548,7 @@ UpdatePlatformInformation (
   MRC_PARAMS_SAVE_RESTORE  *MemInfoHob = NULL;
   UINT32   MrcVersion;
   UINTNIndex;
+  UINT8SecureBoot;
 
   DEBUG ((EFI_D_INFO, "Executing SMBIOS T0x94 Update.\n"));
   //
@@ -666,10 +667,18 @@ UpdatePlatformInformation (
   );
   ASSERT_EFI_ERROR (Status);
 
+  DataSize = sizeof (SecureBoot);
+  Status = gRT->GetVariable (
+  EFI_SECURE_BOOT_MODE_NAME,
+  ,
+  NULL,
+  ,
+  
+  );
   //
   // Secure boot
   //
-  Data8 = SystemConfiguration.SecureBoot;
+  Data8 = SecureBoot;
   UnicodeSPrint (Buffer, sizeof(Buffer), L"%x", Data8);
   HiiSetString (mHiiHandle, STRING_TOKEN (STR_MISC_SECURE_BOOT_VALUE), Buffer, 
NULL);
 
diff --git 
a/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/SmBiosMiscDxe.inf
 
b/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/SmBiosMiscDxe.inf
index 31fd8406b2..6de2d68b57 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/SmBiosMiscDxe.inf
+++ 
b/Platform/BroxtonPlatformPkg/Common/Features/Smbios/SmBiosMiscDxe/SmBiosMiscDxe.inf
@@ -120,6 +120,7 @@
   gIFWIVersionHobGuid
   gEfiPlatformInfoGuid
   gFspNonVolatileStorageHobGuid  ##CONSUMES
+  gEfiGlobalVariableGuid
 
 [Protocols]
   gEfiSmbiosProtocolGuid# PROTOCOL ALWAYS_CONSUMED
diff --git a/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h 
b/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h
index 4f5eeb1c0f..19b948c0ea 100644
--- a/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h
+++ b/Platform/BroxtonPlatformPkg/Common/Include/Guid/SetupVariable.h
@@ -474,7 +474,7 @@ typedef struct {
   UINT8 CapOrVoltFlag;
   UINT8 BootOnInvalidBatt;
   UINT8 ScramblerSupport;
-  UINT8 SecureBoot;
+  UINT8 SecureBootReserved;
   UINT8 SecureBootCustomMode;
   UINT8 MaxPkgCState;
   UINT8 PanelScaling;
diff --git 
a/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
 
b/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
index f56097f3a7..a003b278b4 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/Library/PeiPlatformConfigUpdateLib/PeiPlatformConfigUpdateLib.c
@@ -181,10 +181,9 @@ UpdateSetupDataValues (
   }
 
   if (FdoEnabledGuidHob != NULL) {
-PreDefaultSetupData->SecureBoot = FALSE;
 PreDefaultSetupData->FprrEnable = FALSE;
 PreDefaultSetupData->ScBiosLock = FALSE;
-DEBUG ((EFI_D_INFO, "SPI FDO mode is enabled. Disabling SecureBoot, 
FprrEnable, and ScBiosLock.\n"));
+DEBUG ((EFI_D_INFO, "SPI FDO mode is enabled. Disabling FprrEnable, and 
ScBiosLock.\n"));
   }
 
   Status = GetSecureNfcInfo (PreDefaultSetupData);
diff --git 
a/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformSetupDxe/Boot.vfi 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformSetupDxe/Boot.vfi
index 6b6f262efd..c04d8d6a2a 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformSetupDxe/Boot.vfi
+++ 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformSetupDxe/Boot.vfi
@@ -44,13 +44,6 @@ form formid = BOOT_CONFIGURATION_FORM_ID,
   endoneof;
   endif; // suppressif
 
-  oneof varid  = Setup.SecureBoot,
-prompt   = STRING_TOKEN(STR_SECURITY_BOOT_PROMPT),
-help = STRING_TOKEN(STR_SECURITY_BOOT_HELP),
-option text = STRING_TOKEN(STR_DISABLE), value=0, flags=DEFAULT | 
MANUFACTURING | RESET_REQUIRED;
-option text = STRING_TOKEN(STR_ENABLE),  value=1, flags=0 | RESET_REQUIRED;
-  endoneof;
-
   oneof varid  = 

[edk2] [PATCH v3 1/5] MdePkg/BaseLib: Add new AsmLfence API

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1193

This commit will add a new BaseLib API AsmLfence(). This API will perform
a serializing operation on all load-from-memory instructions that were
issued prior to the call of this function. Please note that this API is
only available on IA-32 and x64.

The purpose of adding this API is to mitigate of the [CVE-2017-5753]
Bounds Check Bypass issue when untrusted data are being processed within
SMM. More details can be referred at the 'Bounds check bypass mitigation'
section at the below link:

https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
Reviewed-by: Jiewen Yao 
Reviewed-by: Liming Gao 
---
 MdePkg/Include/Library/BaseLib.h| 13 +++
 MdePkg/Library/BaseLib/BaseLib.inf  |  2 ++
 MdePkg/Library/BaseLib/Ia32/Lfence.nasm | 36 +++
 MdePkg/Library/BaseLib/X64/Lfence.nasm  | 37 
 4 files changed, 88 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 2fae312f2f..8cc086983d 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -7697,6 +7697,19 @@ AsmWriteTr (
   );
 
 /**
+  Performs a serializing operation on all load-from-memory instructions that
+  were issued prior the AsmLfence function.
+
+  Executes a LFENCE instruction. This function is only available on IA-32 and 
x64.
+
+**/
+VOID
+EFIAPI
+AsmLfence (
+  VOID
+  );
+
+/**
   Patch the immediate operand of an IA32 or X64 instruction such that the byte,
   word, dword or qword operand is encoded at the end of the instruction's
   binary representation.
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf 
b/MdePkg/Library/BaseLib/BaseLib.inf
index 64f6b05741..d971189dff 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -68,6 +68,7 @@
 
 [Sources.Ia32]
   Ia32/WriteTr.nasm
+  Ia32/Lfence.nasm
 
   Ia32/Wbinvd.c | MSFT
   Ia32/WriteMm7.c | MSFT
@@ -346,6 +347,7 @@
   X64/EnableCache.nasm
   X64/DisableCache.nasm
   X64/WriteTr.nasm
+  X64/Lfence.nasm
 
   X64/CpuBreakpoint.c | MSFT
   X64/WriteMsr64.c | MSFT
diff --git a/MdePkg/Library/BaseLib/Ia32/Lfence.nasm 
b/MdePkg/Library/BaseLib/Ia32/Lfence.nasm
new file mode 100644
index 00..414f7d7344
--- /dev/null
+++ b/MdePkg/Library/BaseLib/Ia32/Lfence.nasm
@@ -0,0 +1,36 @@
+;--
 ;
+; Copyright (c) 2018, Intel Corporation. All rights reserved.
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD 
License
+; which accompanies this distribution.  The full text of the license may be 
found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+; Module Name:
+;
+;   Lfence.nasm
+;
+; Abstract:
+;
+;   Performs a serializing operation on all load-from-memory instructions that
+;   were issued prior to the call of this function.
+;
+; Notes:
+;
+;--
+
+SECTION .text
+
+;--
+; VOID
+; EFIAPI
+; AsmLfence (
+;   VOID
+;   );
+;--
+global ASM_PFX(AsmLfence)
+ASM_PFX(AsmLfence):
+lfence
+ret
diff --git a/MdePkg/Library/BaseLib/X64/Lfence.nasm 
b/MdePkg/Library/BaseLib/X64/Lfence.nasm
new file mode 100644
index 00..9ee9b563e2
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/Lfence.nasm
@@ -0,0 +1,37 @@
+;--
 ;
+; Copyright (c) 2018, Intel Corporation. All rights reserved.
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD 
License
+; which accompanies this distribution.  The full text of the license may be 
found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+; Module Name:
+;
+;   Lfence.nasm
+;
+; Abstract:
+;
+;   Performs a serializing operation on all load-from-memory instructions that
+;   were issued prior to the call of this function.
+;
+; Notes:
+;
+;--
+
+DEFAULT REL
+SECTION .text
+
+;--
+; VOID
+; EFIAPI
+; AsmLfence (
+;   VOID
+;   );

[edk2] [PATCH v3 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix bounds check bypass

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194

Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.

This commit will focus on the SMI handler(s) registered within the
FaultTolerantWriteDxe driver and insert AsmLfence API to mitigate the
bounds check bypass issue.

For SMI handler SmmFaultTolerantWriteHandler():

Under "case FTW_FUNCTION_WRITE:", 'SmmFtwWriteHeader->Length' can be a
potential cross boundary access of the 'CommBuffer' (controlled external
inputs) during speculative execution. This cross boundary access is later
passed as parameter 'Length' into function FtwWrite().

Within function FtwWrite(), the value of 'Length' can be inferred by code:
"CopyMem (MyBuffer + Offset, Buffer, Length);". One can observe which part
of the content within 'Buffer' was brought into cache to possibly reveal
the value of 'Length'.

Hence, this commit adds a AsmLfence() after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.

A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
Reviewed-by: Jiewen Yao 
Reviewed-by: Star Zeng 
---
 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c   | 7 
+++
 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf | 1 +
 2 files changed, 8 insertions(+)

diff --git 
a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c 
b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
index 632313f076..27fcab19b6 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
@@ -57,6 +57,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "FaultTolerantWrite.h"
 #include "FaultTolerantWriteSmmCommon.h"
@@ -417,6 +418,12 @@ SmmFaultTolerantWriteHandler (
  
  );
   if (!EFI_ERROR (Status)) {
+//
+// The AsmLfence() call here is to ensure the previous range/content
+// checks for the CommBuffer have been completed before calling into
+// FtwWrite().
+//
+AsmLfence ();
 Status = FtwWrite(
>FtwInstance,
SmmFtwWriteHeader->Lba,
diff --git 
a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf 
b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
index 85d109e8d9..606cc2266b 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
@@ -55,6 +55,7 @@
   PcdLib
   ReportStatusCodeLib
   SmmMemLib
+  BaseLib
 
 [Guids]
   #
-- 
2.12.0.windows.1

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

[edk2] [PATCH v3 5/5] UefiCpuPkg/PiSmmCpuDxeSmm: [CVE-2017-5753] Fix bounds check bypass

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194

Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.

It is possible for SMI handler(s) to call EFI_SMM_CPU_PROTOCOL service
ReadSaveState() and use the content in the 'CommBuffer' (controlled
external inputs) as the 'CpuIndex'. So this commit will insert AsmLfence
API to mitigate the bounds check bypass issue within SmmReadSaveState().

For SmmReadSaveState():

The 'CpuIndex' will be passed into function ReadSaveStateRegister(). And
then in to ReadSaveStateRegisterByIndex().

With the call:
ReadSaveStateRegisterByIndex (
  CpuIndex,
  SMM_SAVE_STATE_REGISTER_IOMISC_INDEX,
  sizeof(IoMisc.Uint32),
  
  );

The 'IoMisc' can be a cross boundary access during speculative execution.
Later, 'IoMisc' is used as the index to access buffers 'mSmmCpuIoWidth'
and 'mSmmCpuIoType'. One can observe which part of the content within
those buffers was brought into cache to possibly reveal the value of
'IoMisc'.

Hence, this commit adds a AsmLfence() after the check of 'CpuIndex'
within function SmmReadSaveState() to prevent the speculative execution.

A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Cc: Laszlo Ersek 
Cc: Michael D Kinney 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
Reviewed-by: Jiewen Yao 
Reviewed-by: Eric Dong 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index fbf74e8d90..19979d5418 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -237,6 +237,11 @@ SmmReadSaveState (
   if ((CpuIndex >= gSmst->NumberOfCpus) || (Buffer == NULL)) {
 return EFI_INVALID_PARAMETER;
   }
+  //
+  // The AsmLfence() call here is to ensure the above check for the CpuIndex
+  // has been completed before the execution of subsequent codes.
+  //
+  AsmLfence ();
 
   //
   // Check for special EFI_SMM_SAVE_STATE_REGISTER_PROCESSOR_ID
-- 
2.12.0.windows.1

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

[edk2] [PATCH v3 3/5] MdeModulePkg/SmmLockBox: [CVE-2017-5753] Fix bounds check bypass

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194

Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.

This commit will focus on the SMI handler(s) registered within the
SmmLockBox driver and insert AsmLfence API to mitigate the
bounds check bypass issue.

For SMI handler SmmLockBoxHandler():

Under "case EFI_SMM_LOCK_BOX_COMMAND_SAVE:", the 'CommBuffer' (controlled
external inputs) is passed to function SmmLockBoxSave().

'TempLockBoxParameterSave.Length' can be a potential cross boundary access
of the 'CommBuffer' during speculative execution. This cross boundary
access is later passed as parameter 'Length' into function SaveLockBox().

Within function SaveLockBox(), the value of 'Length' can be inferred by
code:
"CopyMem ((VOID *)(UINTN)SmramBuffer, (VOID *)(UINTN)Buffer, Length);".
One can observe which part of the content within 'Buffer' was brought into
cache to possibly reveal the value of 'Length'.

Hence, this commit adds a AsmLfence() after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.

And there is a similar case under "case EFI_SMM_LOCK_BOX_COMMAND_UPDATE:"
function SmmLockBoxUpdate() as well. This commits also handles it.

A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
Reviewed-by: Jiewen Yao 
Reviewed-by: Star Zeng 
---
 MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c 
b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c
index 5a11743cb9..c1c9aa5663 100644
--- a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c
+++ b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c
@@ -76,6 +76,11 @@ SmmLockBoxSave (
 LockBoxParameterSave->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED;
 return ;
   }
+  //
+  // The AsmLfence() call here is to ensure the above range check for the
+  // CommBuffer have been completed before calling into SaveLockBox().
+  //
+  AsmLfence ();
 
   //
   // Save data
@@ -160,6 +165,11 @@ SmmLockBoxUpdate (
 LockBoxParameterUpdate->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED;
 return ;
   }
+  //
+  // The AsmLfence() call here is to ensure the above range check for the
+  // CommBuffer have been completed before calling into UpdateLockBox().
+  //
+  AsmLfence ();
 
   //
   // Update data
-- 
2.12.0.windows.1

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

Re: [edk2] [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix bounds check bypass

[Zeng, Star]

Got it, thanks.

Reviewed-by: Star Zeng 

Star
-Original Message-
From: Wu, Hao A 
Sent: Saturday, September 29, 2018 2:21 PM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Yao, Jiewen 
Subject: RE: [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix 
bounds check bypass

> -Original Message-
> From: Zeng, Star
> Sent: Saturday, September 29, 2018 2:11 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Yao, Jiewen; Zeng, Star
> Subject: RE: [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-
> 5753]Fix bounds check bypass
> 
> Please double check whether the AsmLfence calling should be before the 
> line below.
> 
> PrivateData = (VOID *)>Data[Length];

Hi,

The above code is getting the address of a possible cross bounday access during 
the speculative execution.

I also checked that the subsequent usage of 'PrivateData' does not have a code 
pattern of the 'Bounds check bypass' issue. So I think the
AsmLfence() is not needed here.

Best Regards,
Hao Wu

> 
> 
> Thanks,
> Star
> -Original Message-
> From: Wu, Hao A
> Sent: Tuesday, September 25, 2018 2:13 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Yao, Jiewen 
> ; Zeng, Star 
> Subject: [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-
> 5753]Fix bounds check bypass
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
> 
> Speculative execution is used by processor to avoid having to wait for 
> data to arrive from memory, or for previous operations to finish, the 
> processor may speculate as to what will be executed.
> 
> If the speculation is incorrect, the speculatively executed 
> instructions might leave hints such as which memory locations have been 
> brought into cache.
> Malicious actors can use the bounds check bypass method (code gadgets 
> with controlled external inputs) to infer data values that have been 
> used in speculative operations to reveal secrets which should not 
> otherwise be accessed.
> 
> This commit will focus on the SMI handler(s) registered within the 
> FaultTolerantWriteDxe driver and insert AsmLfence API to mitigate the 
> bounds check bypass issue.
> 
> For SMI handler SmmFaultTolerantWriteHandler():
> 
> Under "case FTW_FUNCTION_WRITE:", 'SmmFtwWriteHeader->Length' can be a 
> potential cross boundary access of the 'CommBuffer' (controlled 
> external
> inputs) during speculative execution. This cross boundary access is 
> later passed as parameter 'Length' into function FtwWrite().
> 
> Within function FtwWrite(), the value of 'Length' can be inferred by code:
> "CopyMem (MyBuffer + Offset, Buffer, Length);". One can observe which 
> part of the content within 'Buffer' was brought into cache to possibly 
> reveal the value of 'Length'.
> 
> Hence, this commit adds a AsmLfence() after the boundary/range checks 
> of 'CommBuffer' to prevent the speculative execution.
> 
> A more detailed explanation of the purpose of commit is under the 
> 'Bounds check bypass mitigation' section of the below link:
> https://software.intel.com/security-software-guidance/insights/host-fi
> rmware- speculative-execution-side-channel-mitigation
> 
> And the document at:
> https://software.intel.com/security-software-guidance/api-
> app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass
> -
> vulnerabilities.pdf
> 
> Cc: Jiewen Yao 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu 
> ---
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> | 7 +++
>  
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
> | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> index 632313f076..27fcab19b6 100644
> ---
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> +++
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm
> +++ .c
> @@ -57,6 +57,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> EITHER EXPRESS OR IMPLIED.
>  #include 
>  #include   #include 
> 
> +#include 
>  #include   #include 
> "FaultTolerantWrite.h"
>  #include "FaultTolerantWriteSmmCommon.h"
> @@ -417,6 +418,12 @@ SmmFaultTolerantWriteHandler (
>   
>   );
>if (!EFI_ERROR (Status)) {
> +//
> +// The AsmLfence() call here is to ensure the previous range/content
> +// checks for the CommBuffer have been completed before calling into
> +// FtwWrite().
> +//
> +AsmLfence ();
>  Status = FtwWrite(
> >FtwInstance,
> SmmFtwWriteHeader->Lba, diff --git 
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.i
> n
> f
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.i
> n
> f
> index 85d109e8d9..606cc2266b 100644
> ---
> 

Re: [edk2] [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix bounds check bypass

[Wu, Hao A]

> -Original Message-
> From: Zeng, Star
> Sent: Saturday, September 29, 2018 2:11 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Yao, Jiewen; Zeng, Star
> Subject: RE: [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-
> 5753]Fix bounds check bypass
> 
> Please double check whether the AsmLfence calling should be before the line
> below.
> 
> PrivateData = (VOID *)>Data[Length];

Hi,

The above code is getting the address of a possible cross bounday access
during the speculative execution.

I also checked that the subsequent usage of 'PrivateData' does not have a
code pattern of the 'Bounds check bypass' issue. So I think the
AsmLfence() is not needed here.

Best Regards,
Hao Wu

> 
> 
> Thanks,
> Star
> -Original Message-
> From: Wu, Hao A
> Sent: Tuesday, September 25, 2018 2:13 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Yao, Jiewen ;
> Zeng, Star 
> Subject: [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-
> 5753]Fix bounds check bypass
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
> 
> Speculative execution is used by processor to avoid having to wait for data to
> arrive from memory, or for previous operations to finish, the processor may
> speculate as to what will be executed.
> 
> If the speculation is incorrect, the speculatively executed instructions might
> leave hints such as which memory locations have been brought into cache.
> Malicious actors can use the bounds check bypass method (code gadgets with
> controlled external inputs) to infer data values that have been used in
> speculative operations to reveal secrets which should not otherwise be
> accessed.
> 
> This commit will focus on the SMI handler(s) registered within the
> FaultTolerantWriteDxe driver and insert AsmLfence API to mitigate the bounds
> check bypass issue.
> 
> For SMI handler SmmFaultTolerantWriteHandler():
> 
> Under "case FTW_FUNCTION_WRITE:", 'SmmFtwWriteHeader->Length' can be
> a potential cross boundary access of the 'CommBuffer' (controlled external
> inputs) during speculative execution. This cross boundary access is later 
> passed
> as parameter 'Length' into function FtwWrite().
> 
> Within function FtwWrite(), the value of 'Length' can be inferred by code:
> "CopyMem (MyBuffer + Offset, Buffer, Length);". One can observe which part
> of the content within 'Buffer' was brought into cache to possibly reveal the
> value of 'Length'.
> 
> Hence, this commit adds a AsmLfence() after the boundary/range checks of
> 'CommBuffer' to prevent the speculative execution.
> 
> A more detailed explanation of the purpose of commit is under the 'Bounds
> check bypass mitigation' section of the below link:
> https://software.intel.com/security-software-guidance/insights/host-firmware-
> speculative-execution-side-channel-mitigation
> 
> And the document at:
> https://software.intel.com/security-software-guidance/api-
> app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-
> vulnerabilities.pdf
> 
> Cc: Jiewen Yao 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu 
> ---
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> | 7 +++
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
> | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> index 632313f076..27fcab19b6 100644
> ---
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> +++
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm
> +++ .c
> @@ -57,6 +57,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include 
>  #include   #include 
> +#include 
>  #include   #include
> "FaultTolerantWrite.h"
>  #include "FaultTolerantWriteSmmCommon.h"
> @@ -417,6 +418,12 @@ SmmFaultTolerantWriteHandler (
>   
>   );
>if (!EFI_ERROR (Status)) {
> +//
> +// The AsmLfence() call here is to ensure the previous range/content
> +// checks for the CommBuffer have been completed before calling into
> +// FtwWrite().
> +//
> +AsmLfence ();
>  Status = FtwWrite(
> >FtwInstance,
> SmmFtwWriteHeader->Lba, diff --git
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.in
> f
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.in
> f
> index 85d109e8d9..606cc2266b 100644
> ---
> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.in
> f
> +++
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm
> +++ .inf
> @@ -55,6 +55,7 @@
>PcdLib
>ReportStatusCodeLib
>SmmMemLib
> +  BaseLib
> 
>  [Guids]
>#
> --
> 2.12.0.windows.1

___
edk2-devel 

Re: [edk2] [PATCH v2 4/5] MdeModulePkg/Variable: [CVE-2017-5753] Fix bounds check bypass

[Zeng, Star]

VariableSmm.inf is including LoadFenceDxe.c, it should be LoadFenceSmm.c.

I also suggest using MemoryLoadFence instead of VariableLoadFence as the name.


With them corrected, Reviewed-by: Star Zeng .

Thanks,
Star
-Original Message-
From: Wu, Hao A 
Sent: Tuesday, September 25, 2018 2:13 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Yao, Jiewen ; Zeng, 
Star 
Subject: [PATCH v2 4/5] MdeModulePkg/Variable: [CVE-2017-5753] Fix bounds check 
bypass

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194

Speculative execution is used by processor to avoid having to wait for data to 
arrive from memory, or for previous operations to finish, the processor may 
speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions might 
leave hints such as which memory locations have been brought into cache. 
Malicious actors can use the bounds check bypass method (code gadgets with 
controlled external inputs) to infer data values that have been used in 
speculative operations to reveal secrets which should not otherwise be accessed.

This commit will focus on the SMI handler(s) registered within the 
Variable\RuntimeDxe driver and insert AsmLfence API to mitigate the bounds 
check bypass issue.

For SMI handler SmmVariableHandler():

Under "case SMM_VARIABLE_FUNCTION_GET_VARIABLE:",
'SmmVariableHeader->NameSize' can be a potential cross boundary access of the 
'CommBuffer' (controlled external input) during speculative execution.

This cross boundary access is later used as the index to access array 
'SmmVariableHeader->Name' by code:
"SmmVariableHeader->Name[SmmVariableHeader->NameSize/sizeof (CHAR16) - 1]"
One can observe which part of the content within array was brought into cache 
to possibly reveal the value of 'SmmVariableHeader->NameSize'.

Hence, this commit adds a AsmLfence() after the boundary/range checks of 
'CommBuffer' to prevent the speculative execution.

And there are 2 similar cases under
"case SMM_VARIABLE_FUNCTION_SET_VARIABLE:" and "case 
SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET:" as well.
This commits also handles them.

Also, under "case SMM_VARIABLE_FUNCTION_SET_VARIABLE:",
'(UINT8 *)SmmVariableHeader->Name + SmmVariableHeader->NameSize' points to the 
'CommBuffer' (with some offset) and then passed as parameter 'Data' to function 
VariableServiceSetVariable().

Within function VariableServiceSetVariable(), there is a sanity check for
EFI_VARIABLE_AUTHENTICATION_2 descriptor for the data pointed by 'Data'.
If this check is speculatively bypassed, potential cross-boundary data access 
for 'Data' is possible to be revealed via the below function calls sequence 
during speculative execution:

AuthVariableLibProcessVariable()
ProcessVarWithPk() or ProcessVarWithKek()

Within function ProcessVarWithPk() or ProcessVarWithKek(), for the code 
"PayloadSize = DataSize - AUTHINFO2_SIZE (Data);", 'AUTHINFO2_SIZE (Data)'
can be a cross boundary access during speculative execution.

Then, 'PayloadSize' is possible to be revealed by the function call
sequence:

AuthServiceInternalUpdateVariableWithTimeStamp()
mAuthVarLibContextIn->UpdateVariable()
VariableExLibUpdateVariable()
UpdateVariable()
CopyMem()

Hence, this commit adds a AsmLfence() after the sanity check for
EFI_VARIABLE_AUTHENTICATION_2 descriptor upon 'Data' within function
VariableServiceSetVariable() to prevent the speculative execution.

Also, please note that the change made within function
VariableServiceSetVariable() will affect DXE as well. However, since we only 
focuses on the SMM codes, the commit will introduce a new module internal 
function called VariableLoadFence() to handle this. This internal function will 
have 2 implementations (1 for SMM, 1 for DXE). For the SMM implementation, it 
is a wrapper to call the AsmLfence() API; for the DXE implementation, it is 
empty.

A more detailed explanation of the purpose of commit is under the 'Bounds check 
bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Cc: Jiewen Yao 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceDxe.c | 31 

 MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceSmm.c | 30 
+++
 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 13 +++-
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c |  6 
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf |  1 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c  | 18 

 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf   

Re: [edk2] [PATCH v2 3/5] MdeModulePkg/SmmLockBox: [CVE-2017-5753] Fix bounds check bypass

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Wu, Hao A 
Sent: Tuesday, September 25, 2018 2:13 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Yao, Jiewen ; Zeng, 
Star 
Subject: [PATCH v2 3/5] MdeModulePkg/SmmLockBox: [CVE-2017-5753] Fix bounds 
check bypass

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194

Speculative execution is used by processor to avoid having to wait for data to 
arrive from memory, or for previous operations to finish, the processor may 
speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions might 
leave hints such as which memory locations have been brought into cache. 
Malicious actors can use the bounds check bypass method (code gadgets with 
controlled external inputs) to infer data values that have been used in 
speculative operations to reveal secrets which should not otherwise be accessed.

This commit will focus on the SMI handler(s) registered within the SmmLockBox 
driver and insert AsmLfence API to mitigate the bounds check bypass issue.

For SMI handler SmmLockBoxHandler():

Under "case EFI_SMM_LOCK_BOX_COMMAND_SAVE:", the 'CommBuffer' (controlled 
external inputs) is passed to function SmmLockBoxSave().

'TempLockBoxParameterSave.Length' can be a potential cross boundary access of 
the 'CommBuffer' during speculative execution. This cross boundary access is 
later passed as parameter 'Length' into function SaveLockBox().

Within function SaveLockBox(), the value of 'Length' can be inferred by
code:
"CopyMem ((VOID *)(UINTN)SmramBuffer, (VOID *)(UINTN)Buffer, Length);".
One can observe which part of the content within 'Buffer' was brought into 
cache to possibly reveal the value of 'Length'.

Hence, this commit adds a AsmLfence() after the boundary/range checks of 
'CommBuffer' to prevent the speculative execution.

And there is a similar case under "case EFI_SMM_LOCK_BOX_COMMAND_UPDATE:"
function SmmLockBoxUpdate() as well. This commits also handles it.

A more detailed explanation of the purpose of commit is under the 'Bounds check 
bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Cc: Jiewen Yao 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c 
b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c
index 5a11743cb9..c1c9aa5663 100644
--- a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c
+++ b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c
@@ -76,6 +76,11 @@ SmmLockBoxSave (
 LockBoxParameterSave->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED;
 return ;
   }
+  //
+  // The AsmLfence() call here is to ensure the above range check for 
+ the  // CommBuffer have been completed before calling into SaveLockBox().
+  //
+  AsmLfence ();
 
   //
   // Save data
@@ -160,6 +165,11 @@ SmmLockBoxUpdate (
 LockBoxParameterUpdate->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED;
 return ;
   }
+  //
+  // The AsmLfence() call here is to ensure the above range check for 
+ the  // CommBuffer have been completed before calling into UpdateLockBox().
+  //
+  AsmLfence ();
 
   //
   // Update data
--
2.12.0.windows.1

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

Re: [edk2] [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix bounds check bypass

[Zeng, Star]

Please double check whether the AsmLfence calling should be before the line 
below.

PrivateData = (VOID *)>Data[Length];


Thanks,
Star
-Original Message-
From: Wu, Hao A 
Sent: Tuesday, September 25, 2018 2:13 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Yao, Jiewen ; Zeng, 
Star 
Subject: [PATCH v2 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix 
bounds check bypass

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194

Speculative execution is used by processor to avoid having to wait for data to 
arrive from memory, or for previous operations to finish, the processor may 
speculate as to what will be executed.

If the speculation is incorrect, the speculatively executed instructions might 
leave hints such as which memory locations have been brought into cache. 
Malicious actors can use the bounds check bypass method (code gadgets with 
controlled external inputs) to infer data values that have been used in 
speculative operations to reveal secrets which should not otherwise be accessed.

This commit will focus on the SMI handler(s) registered within the 
FaultTolerantWriteDxe driver and insert AsmLfence API to mitigate the bounds 
check bypass issue.

For SMI handler SmmFaultTolerantWriteHandler():

Under "case FTW_FUNCTION_WRITE:", 'SmmFtwWriteHeader->Length' can be a 
potential cross boundary access of the 'CommBuffer' (controlled external
inputs) during speculative execution. This cross boundary access is later 
passed as parameter 'Length' into function FtwWrite().

Within function FtwWrite(), the value of 'Length' can be inferred by code:
"CopyMem (MyBuffer + Offset, Buffer, Length);". One can observe which part of 
the content within 'Buffer' was brought into cache to possibly reveal the value 
of 'Length'.

Hence, this commit adds a AsmLfence() after the boundary/range checks of 
'CommBuffer' to prevent the speculative execution.

A more detailed explanation of the purpose of commit is under the 'Bounds check 
bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation

And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf

Cc: Jiewen Yao 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c   | 7 
+++
 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf | 1 +
 2 files changed, 8 insertions(+)

diff --git 
a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c 
b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
index 632313f076..27fcab19b6 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm
+++ .c
@@ -57,6 +57,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include   #include 
+#include 
 #include   #include "FaultTolerantWrite.h"
 #include "FaultTolerantWriteSmmCommon.h"
@@ -417,6 +418,12 @@ SmmFaultTolerantWriteHandler (
  
  );
   if (!EFI_ERROR (Status)) {
+//
+// The AsmLfence() call here is to ensure the previous range/content
+// checks for the CommBuffer have been completed before calling into
+// FtwWrite().
+//
+AsmLfence ();
 Status = FtwWrite(
>FtwInstance,
SmmFtwWriteHeader->Lba, diff --git 
a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf 
b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
index 85d109e8d9..606cc2266b 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm
+++ .inf
@@ -55,6 +55,7 @@
   PcdLib
   ReportStatusCodeLib
   SmmMemLib
+  BaseLib
 
 [Guids]
   #
--
2.12.0.windows.1

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