Legacy has to access interrupt vector, BDA, etc. located in memory between
0-4095. To allow as much code as possible to be monitored by NULL pointer
detection, we add code to temporarily disable this feature right before
those memory access and enable it again afterwards.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Ayellet Wolman <ayellet.wolman@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c       | 101 ++++++++++++++
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h       |   2 +
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf      |   2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c                  |   4 +
 .../Csm/LegacyBiosDxe/LegacyBios.c                 | 152 +++++++++++++++++++++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf            |   2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h        |  18 +++
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c          |  23 +++-
 .../Csm/LegacyBiosDxe/LegacyPci.c                  |  17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  |  27 +++-
 10 files changed, 338 insertions(+), 10 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
index 7308523ad8..d2224a20aa 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
@@ -1732,6 +1732,98 @@ CheckKeyboardConnect (
   }
 }
 
+/**
+   Disable NULL pointer detection
+*/
+VOID
+DisableNullDetection (
+  VOID
+  )
+{
+  EFI_STATUS                            Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc;
+
+  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+    return;
+  }
+
+  //
+  // Check current capabilities and attributes
+  //
+  Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Try to add EFI_MEMORY_RP support if necessary
+  //
+  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+    Desc.Capabilities |= EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+                                              Desc.Capabilities);
+    ASSERT_EFI_ERROR (Status);
+    if (EFI_ERROR (Status)) {
+      return;
+    }
+  }
+
+  //
+  // Don't bother if EFI_MEMORY_RP is already cleared.
+  //
+  if ((Desc.Attributes & EFI_MEMORY_RP) != 0) {
+    Desc.Attributes &= ~EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+                                            Desc.Attributes);
+    ASSERT_EFI_ERROR (Status);
+  } else {
+    DEBUG ((DEBUG_WARN, "!!! Page 0 is supposed to be disabled !!!\r\n"));
+  }
+}
+
+/**
+   Enable NULL pointer detection
+*/
+VOID
+EnableNullDetection (
+  VOID
+  )
+{
+  EFI_STATUS                            Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc;
+
+  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+    return;
+  }
+
+  //
+  // Check current capabilities and attributes
+  //
+  Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Try to add EFI_MEMORY_RP support if necessary
+  //
+  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+    Desc.Capabilities |= EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+                                              Desc.Capabilities);
+    ASSERT_EFI_ERROR (Status);
+    if (EFI_ERROR (Status)) {
+      return;
+    }
+  }
+
+  //
+  // Don't bother if EFI_MEMORY_RP is already set.
+  //
+  if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
+    Desc.Attributes |= EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+                                            Desc.Attributes);
+    ASSERT_EFI_ERROR (Status);
+  }
+}
+
 /**
   Timer event handler: read a series of key stroke from 8042
   and put them into memory key buffer. 
@@ -1839,6 +1931,11 @@ BiosKeyboardTimerHandler (
   //   0 Right Shift pressed
 
 
+  //
+  // Disable NULL pointer detection temporarily
+  //
+  DisableNullDetection ();
+
   //
   // Clear the CTRL and ALT BDA flag
   //
@@ -1916,6 +2013,10 @@ BiosKeyboardTimerHandler (
   KbFlag1 &= ~0x0C;                      
   *((UINT8 *) (UINTN) 0x417) = KbFlag1; 
 
+  //
+  // Restore NULL pointer detection
+  //
+  EnableNullDetection ();
   
   //
   // Output EFI input key and shift/toggle state
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
index 0bf28ea140..c64ec0095e 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
@@ -18,6 +18,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 
 #include <FrameworkDxe.h>
+#include <Pi/PiDxeCis.h>
 
 #include <Guid/StatusCodeDataTypeId.h>
 #include <Protocol/SimpleTextIn.h>
@@ -33,6 +34,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/ReportStatusCodeLib.h>
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/DxeServicesTableLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/BaseLib.h>
 #include <Library/PcdLib.h>
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
index 4d4536466c..596f4ced44 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
@@ -60,6 +60,7 @@
   DebugLib
   BaseLib
   PcdLib
+  DxeServicesTableLib
 
 [Protocols]
   gEfiIsaIoProtocolGuid                         ## TO_START
@@ -73,6 +74,7 @@
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFastPS2Detection                  ## SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask  ## CONSUMES
 
 [UserExtensions.TianoCore."ExtraFiles"]
   KeyboardDxeExtra.uni
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
index c45d5d4c3e..c6670febee 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
@@ -34,6 +34,8 @@ LegacyBiosInitBda (
   BDA_STRUC *Bda;
   UINT8     *Ebda;
 
+  DisableNullDetection ();
+
   Bda   = (BDA_STRUC *) ((UINTN) 0x400);
   Ebda  = (UINT8 *) ((UINTN) 0x9fc00);
 
@@ -62,5 +64,7 @@ LegacyBiosInitBda (
 
   *Ebda               = 0x01;
 
+  EnableNullDetection ();
+
   return EFI_SUCCESS;
 }
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
index 3ead2d9828..3176a989f6 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -40,6 +40,7 @@ VOID                  *mRuntimeSmbiosEntryPoint = NULL;
 EFI_PHYSICAL_ADDRESS  mReserveSmbiosEntryPoint = 0;
 EFI_PHYSICAL_ADDRESS  mStructureTableAddress   = 0;
 UINTN                 mStructureTablePages     = 0;
+BOOLEAN               mEndOfDxe                = FALSE;
 
 /**
   Do an AllocatePages () of type AllocateMaxAddress for EfiBootServicesCode
@@ -765,6 +766,135 @@ InstallSmbiosEventCallback (
   }
 }
 
+/**
+  Callback function to toggle EndOfDxe status. NULL pointer detection needs
+  this status to decide if it's necessary to change attributes of page 0.
+
+  @param  Event            Event whose notification function is being invoked.
+  @param  Context          The pointer to the notification function's context,
+                           which is implementation-dependent.
+
+**/
+VOID
+EFIAPI
+ToggleEndOfDxeStatus (
+  IN EFI_EVENT                Event,
+  IN VOID                     *Context
+  )
+{
+  mEndOfDxe = TRUE;
+  return;
+}
+
+//
+// Legacy BIOS needs to access memory between 0-4095, which will cause page
+// fault exception if NULL pointer detection mechanism is enabled. Following
+// functions can be used to disable/enable NULL pointer detection before/after
+// accessing those memory.
+//
+
+/**
+   Enable NULL pointer detection
+*/
+VOID
+EnableNullDetection (
+  VOID
+  )
+{
+  EFI_STATUS                            Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc;
+
+  if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
+      ||
+      ((mEndOfDxe == TRUE)  &&
+       ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
+        == (BIT7|BIT0)))
+     ) {
+    return;
+  }
+
+  //
+  // Check current capabilities and attributes
+  //
+  Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Try to add EFI_MEMORY_RP support if necessary
+  //
+  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+    Desc.Capabilities |= EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+                                              Desc.Capabilities);
+    ASSERT_EFI_ERROR (Status);
+    if (EFI_ERROR (Status)) {
+      return;
+    }
+  }
+
+  //
+  // Don't bother if EFI_MEMORY_RP is already set.
+  //
+  if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
+    Desc.Attributes |= EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+                                            Desc.Attributes);
+    ASSERT_EFI_ERROR (Status);
+  }
+}
+
+/**
+   Disable NULL pointer detection
+*/
+VOID
+DisableNullDetection (
+  VOID
+  )
+{
+  EFI_STATUS                            Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc;
+
+  if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
+      ||
+      ((mEndOfDxe == TRUE)  &&
+       ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
+        == (BIT7|BIT0)))
+     ) {
+    return;
+  }
+
+  //
+  // Check current capabilities and attributes
+  //
+  Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Try to add EFI_MEMORY_RP support if necessary
+  //
+  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+    Desc.Capabilities |= EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+                                              Desc.Capabilities);
+    ASSERT_EFI_ERROR (Status);
+    if (EFI_ERROR (Status)) {
+      return;
+    }
+  }
+
+  //
+  // Don't bother if EFI_MEMORY_RP is already cleared.
+  //
+  if ((Desc.Attributes & EFI_MEMORY_RP) != 0) {
+    Desc.Attributes &= ~EFI_MEMORY_RP;
+    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+                                            Desc.Attributes);
+    ASSERT_EFI_ERROR (Status);
+  } else {
+    DEBUG ((DEBUG_WARN, "!!! Page 0 is supposed to be disabled !!!\r\n"));
+  }
+}
+
 /**
   Install Driver to produce Legacy BIOS protocol.
 
@@ -802,6 +932,7 @@ LegacyBiosInstall (
   UINT64                             Length;
   UINT8                              *SecureBoot;
   EFI_EVENT                          InstallSmbiosEvent;
+  EFI_EVENT                          EndOfDxeEvent;
 
   //
   // Load this driver's image to memory
@@ -964,8 +1095,10 @@ LegacyBiosInstall (
   // Initialize region from 0x0000 to 4k. This initializes interrupt vector
   // range.
   //
+  DisableNullDetection ();
   gBS->SetMem ((VOID *) ClearPtr, 0x400, INITIAL_VALUE_BELOW_1K);
   ZeroMem ((VOID *) ((UINTN)ClearPtr + 0x400), 0xC00);
+  EnableNullDetection ();
 
   //
   // Allocate pages for OPROM usage
@@ -1104,12 +1237,17 @@ LegacyBiosInstall (
   //
   // Save Unexpected interrupt vector so can restore it just prior to boot
   //
+  DisableNullDetection ();
+
   BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
   Private->BiosUnexpectedInt = BaseVectorMaster[0];
   IntRedirCode = (UINT32) (UINTN) Private->IntThunk->InterruptRedirectionCode;
   for (Index = 0; Index < 8; Index++) {
     BaseVectorMaster[Index] = (EFI_SEGMENT (IntRedirCode + Index * 4) << 16) | EFI_OFFSET (IntRedirCode + Index * 4);
   }
+
+  EnableNullDetection ();
+
   //
   // Save EFI value
   //
@@ -1133,6 +1271,20 @@ LegacyBiosInstall (
                   );
   ASSERT_EFI_ERROR (Status);  
 
+  //
+  // Create callback to update status of EndOfDxe, which is needed by NULL
+  // pointer detection
+  //
+  Status = gBS->CreateEventEx (
+                  EVT_NOTIFY_SIGNAL,
+                  TPL_NOTIFY,
+                  ToggleEndOfDxeStatus,
+                  NULL,
+                  &gEfiEndOfDxeEventGroupGuid,
+                  &EndOfDxeEvent
+                  );
+  ASSERT_EFI_ERROR (Status);
+
   //
   // Make a new handle and install the protocol
   //
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
index 48473a0713..6efc7f36ae 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
@@ -108,6 +108,7 @@
   gEfiDiskInfoIdeInterfaceGuid                  ## SOMETIMES_CONSUMES ##GUID #Used in LegacyBiosBuildIdeData() to assure device is a disk
   gEfiSmbiosTableGuid                           ## SOMETIMES_CONSUMES ##SystemTable
   gEfiLegacyBiosGuid                            ## SOMETIMES_CONSUMES ##GUID #Used in LegacyBiosInstallVgaRom() to locate handle buffer
+  gEfiEndOfDxeEventGroupGuid                    ## CONSUMES
 
 [Guids.IA32]
   gEfiAcpi20TableGuid                           ## SOMETIMES_CONSUMES ##SystemTable
@@ -147,6 +148,7 @@
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdHighPmmMemorySize            ## CONSUMES
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemoryBase      ## CONSUMES
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemorySize      ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
 
 [Depex]
   gEfiLegacyRegion2ProtocolGuid AND gEfiLegacyInterruptProtocolGuid AND gEfiLegacyBiosPlatformProtocolGuid AND gEfiLegacy8259ProtocolGuid AND gEfiGenericMemTestProtocolGuid AND gEfiCpuArchProtocolGuid AND gEfiTimerArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
index fe9dd7463a..20dfef3fec 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
@@ -509,6 +509,8 @@ extern BBS_TABLE           *mBbsTable;
 
 extern EFI_GENERIC_MEMORY_TEST_PROTOCOL *gGenMemoryTest;
 
+extern BOOLEAN mEndOfDxe;
+
 #define PORT_70 0x70
 #define PORT_71 0x71
 
@@ -1542,4 +1544,20 @@ LegacyBiosInstallVgaRom (
   IN  LEGACY_BIOS_INSTANCE            *Private
   );
 
+/**
+   Enable NULL pointer detection
+*/
+VOID
+EnableNullDetection (
+  VOID
+  );
+
+/**
+   Disable NULL pointer detection
+*/
+VOID
+DisableNullDetection (
+  VOID
+  );
+
 #endif
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
index 1e098b3726..c2ac69ce69 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
@@ -1073,8 +1073,10 @@ GenericLegacyBoot (
   // Use 182/10 to avoid floating point math.
   //
   LocalTime = (LocalTime * 182) / 10;
+  DisableNullDetection ();
   BdaPtr    = (UINT32 *) (UINTN)0x46C;
   *BdaPtr   = LocalTime;
+  EnableNullDetection ();
 
   //
   // Shadow PCI ROMs. We must do this near the end since this will kick
@@ -1320,6 +1322,7 @@ GenericLegacyBoot (
     //          set of TIANO vectors) or takes it over.
     //
     //
+    DisableNullDetection ();
     BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
     for (Index = 0; Index < 8; Index++) {
       Private->ThunkSavedInt[Index] = BaseVectorMaster[Index];
@@ -1327,6 +1330,7 @@ GenericLegacyBoot (
         BaseVectorMaster[Index] = (UINT32) (Private->BiosUnexpectedInt);
       }
     }
+    EnableNullDetection ();
 
     ZeroMem (&Regs, sizeof (EFI_IA32_REGISTER_SET));
     Regs.X.AX = Legacy16Boot;
@@ -1340,10 +1344,12 @@ GenericLegacyBoot (
       0
       );
 
+    DisableNullDetection ();
     BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
     for (Index = 0; Index < 8; Index++) {
       BaseVectorMaster[Index] = Private->ThunkSavedInt[Index];
     }
+    EnableNullDetection ();
   }
   Private->LegacyBootEntered = TRUE;
   if ((mBootMode == BOOT_LEGACY_OS) || (mBootMode == BOOT_UNCONVENTIONAL_DEVICE)) {
@@ -1731,9 +1737,11 @@ LegacyBiosBuildE820 (
   //
   // First entry is 0 to (640k - EBDA)
   //
+  DisableNullDetection ();
   E820Table[0].BaseAddr  = 0;
   E820Table[0].Length    = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
   E820Table[0].Type      = EfiAcpiAddressRangeMemory;
+  EnableNullDetection ();
 
   //
   // Second entry is (640k - EBDA) to 640k
@@ -1967,6 +1975,8 @@ LegacyBiosCompleteBdaBeforeBoot (
   UINT16                      MachineConfig;
   DEVICE_PRODUCER_DATA_HEADER *SioPtr;
 
+  DisableNullDetection ();
+
   Bda           = (BDA_STRUC *) ((UINTN) 0x400);
   MachineConfig = 0;
 
@@ -2025,6 +2035,8 @@ LegacyBiosCompleteBdaBeforeBoot (
   MachineConfig       = (UINT16) (MachineConfig + 0x00 + 0x02 + (SioPtr->MousePresent * 0x04));
   Bda->MachineConfig  = MachineConfig;
 
+  EnableNullDetection ();
+
   return EFI_SUCCESS;
 }
 
@@ -2049,15 +2061,20 @@ LegacyBiosUpdateKeyboardLedStatus (
   UINT8                 LocalLeds;
   EFI_IA32_REGISTER_SET Regs;
 
-  Bda                 = (BDA_STRUC *) ((UINTN) 0x400);
-
   Private             = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
+
+  DisableNullDetection ();
+
+  Bda                 = (BDA_STRUC *) ((UINTN) 0x400);
   LocalLeds           = Leds;
   Bda->LedStatus      = (UINT8) ((Bda->LedStatus &~0x07) | LocalLeds);
   LocalLeds           = (UINT8) (LocalLeds << 4);
   Bda->ShiftStatus    = (UINT8) ((Bda->ShiftStatus &~0x70) | LocalLeds);
   LocalLeds           = (UINT8) (Leds & 0x20);
   Bda->KeyboardStatus = (UINT8) ((Bda->KeyboardStatus &~0x20) | LocalLeds);
+
+  EnableNullDetection ();
+
   //
   // Call into Legacy16 code to allow it to do any processing
   //
@@ -2102,7 +2119,9 @@ LegacyBiosCompleteStandardCmosBeforeBoot (
   //            to large capacity drives
   // CMOS 14 = BDA 40:10 plus bit 3(display enabled)
   //
+  DisableNullDetection ();
   Bda = (UINT8)(*((UINT8 *)((UINTN)0x410)) | BIT3);
+  EnableNullDetection ();
 
   //
   // Force display enabled
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
index 8ffdf0c1ff..d38cef3e33 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
@@ -2279,6 +2279,7 @@ LegacyBiosInstallRom (
   UINTN                 Function;
   EFI_IA32_REGISTER_SET Regs;
   UINT8                 VideoMode;
+  UINT8                 OldVideoMode;
   EFI_TIME              BootTime;
   UINT32                *BdaPtr;
   UINT32                LocalTime;
@@ -2299,6 +2300,7 @@ LegacyBiosInstallRom (
   Device          = 0;
   Function        = 0;
   VideoMode       = 0;
+  OldVideoMode    = 0;
   PhysicalAddress = 0;
   MaxRomAddr      = PcdGet32 (PcdEndOpromShadowAddress);
 
@@ -2401,6 +2403,8 @@ LegacyBiosInstallRom (
   // 2. BBS compliants drives will not change 40:75 until boot time.
   // 3. Onboard IDE controllers will change 40:75
   //
+  DisableNullDetection ();
+
   LocalDiskStart = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
   if ((Private->Disk4075 + 0x80) < LocalDiskStart) {
     //
@@ -2426,6 +2430,9 @@ LegacyBiosInstallRom (
     //
     VideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
   }
+
+  EnableNullDetection ();
+
   //
   // Notify the platform that we are about to scan the ROM
   //
@@ -2466,9 +2473,11 @@ LegacyBiosInstallRom (
   // Multiply result by 18.2 for number of ticks since midnight.
   // Use 182/10 to avoid floating point math.
   //
+  DisableNullDetection ();
   LocalTime = (LocalTime * 182) / 10;
   BdaPtr    = (UINT32 *) ((UINTN) 0x46C);
   *BdaPtr   = LocalTime;
+  EnableNullDetection ();
   
   //
   // Pass in handoff data
@@ -2564,7 +2573,11 @@ LegacyBiosInstallRom (
     //
     // Set mode settings since PrepareToScanRom may change mode
     //
-    if (VideoMode != *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE))) {
+    DisableNullDetection ();
+    OldVideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
+    EnableNullDetection ();
+
+    if (VideoMode != OldVideoMode) {
       //
       // The active video mode is changed, restore it to original mode.
       //
@@ -2604,7 +2617,9 @@ LegacyBiosInstallRom (
     }
   }
 
+  DisableNullDetection ();
   LocalDiskEnd = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
+  EnableNullDetection ();
   
   //
   // Allow platform to perform any required actions after the
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
index 3d9a8b9649..f42c13cd89 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
@@ -57,7 +57,11 @@ LegacyBiosInt86 (
   IN  EFI_IA32_REGISTER_SET         *Regs
   )
 {
-  UINT32  *VectorBase;
+  UINT16                Segment;
+  UINT16                Offset;
+  LEGACY_BIOS_INSTANCE  *Private;
+
+  Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
 
   Regs->X.Flags.Reserved1 = 1;
   Regs->X.Flags.Reserved2 = 0;
@@ -72,12 +76,15 @@ LegacyBiosInt86 (
   // The base address of legacy interrupt vector table is 0.
   // We use this base address to get the legacy interrupt handler.
   //
-  VectorBase              = 0;
+  DisableNullDetection ();
+  Segment               = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
+  Offset                = (UINT16)((UINT32 *)0)[BiosInt];
+  EnableNullDetection ();
   
   return InternalLegacyBiosFarCall (
            This,
-           (UINT16) ((VectorBase)[BiosInt] >> 16),
-           (UINT16) (VectorBase)[BiosInt],
+           Segment,
+           Offset,
            Regs,
            &Regs->X.Flags,
            sizeof (Regs->X.Flags)
@@ -293,9 +300,15 @@ InternalLegacyBiosFarCall (
       UINTN                 EbdaBaseAddress;
       UINTN                 ReservedEbdaBaseAddress;
 
-      EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
-      ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP - PcdGet32 (PcdEbdaReservedMemorySize);
-      ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
+      //
+      // Skip this part of debug code if NULL pointer detection is enabled
+      //
+      if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+        EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
+        ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP
+                                  - PcdGet32 (PcdEbdaReservedMemorySize);
+        ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
+      }
     }
   );
 
-- 
2.14.1.windows.1

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

This patch breaks the GCC5 build:

On 10/09/17 16:17, Jian J Wang wrote:

> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> index 3d9a8b9649..f42c13cd89 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> @@ -57,7 +57,11 @@ LegacyBiosInt86 (
>    IN  EFI_IA32_REGISTER_SET         *Regs
>    )
>  {
> -  UINT32  *VectorBase;
> +  UINT16                Segment;
> +  UINT16                Offset;
> +  LEGACY_BIOS_INSTANCE  *Private;
> +
> +  Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
>  
>    Regs->X.Flags.Reserved1 = 1;
>    Regs->X.Flags.Reserved2 = 0;
> @@ -72,12 +76,15 @@ LegacyBiosInt86 (
>    // The base address of legacy interrupt vector table is 0.
>    // We use this base address to get the legacy interrupt handler.
>    //
> -  VectorBase              = 0;
> +  DisableNullDetection ();
> +  Segment               = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
> +  Offset                = (UINT16)((UINT32 *)0)[BiosInt];
> +  EnableNullDetection ();
>    
>    return InternalLegacyBiosFarCall (
>             This,
> -           (UINT16) ((VectorBase)[BiosInt] >> 16),
> -           (UINT16) (VectorBase)[BiosInt],
> +           Segment,
> +           Offset,
>             Regs,
>             &Regs->X.Flags,
>             sizeof (Regs->X.Flags)

IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c: In function 'LegacyBiosInt86':
IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c:62:26: error: variable 'Private' set but not used [-Werror=unused-but-set-variable]
   LEGACY_BIOS_INSTANCE  *Private;
                          ^~~~~~~
cc1: all warnings being treated as errors

Can you please send a patch?

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

Thanks for catching this issue. Patch has been sent out. 

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, October 12, 2017 5:30 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet
> <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH v4 5/6] IntelFrameworkModulePkg/Csm: Add code
> to bypass NULL pointer detection
> 
> This patch breaks the GCC5 build:
> 
> On 10/09/17 16:17, Jian J Wang wrote:
> 
> > diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> > index 3d9a8b9649..f42c13cd89 100644
> > --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> > +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> > @@ -57,7 +57,11 @@ LegacyBiosInt86 (
> >    IN  EFI_IA32_REGISTER_SET         *Regs
> >    )
> >  {
> > -  UINT32  *VectorBase;
> > +  UINT16                Segment;
> > +  UINT16                Offset;
> > +  LEGACY_BIOS_INSTANCE  *Private;
> > +
> > +  Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
> >
> >    Regs->X.Flags.Reserved1 = 1;
> >    Regs->X.Flags.Reserved2 = 0;
> > @@ -72,12 +76,15 @@ LegacyBiosInt86 (
> >    // The base address of legacy interrupt vector table is 0.
> >    // We use this base address to get the legacy interrupt handler.
> >    //
> > -  VectorBase              = 0;
> > +  DisableNullDetection ();
> > +  Segment               = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
> > +  Offset                = (UINT16)((UINT32 *)0)[BiosInt];
> > +  EnableNullDetection ();
> >
> >    return InternalLegacyBiosFarCall (
> >             This,
> > -           (UINT16) ((VectorBase)[BiosInt] >> 16),
> > -           (UINT16) (VectorBase)[BiosInt],
> > +           Segment,
> > +           Offset,
> >             Regs,
> >             &Regs->X.Flags,
> >             sizeof (Regs->X.Flags)
> 
> IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c: In function
> 'LegacyBiosInt86':
> IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c:62:26: error: variable
> 'Private' set but not used [-Werror=unused-but-set-variable]
>    LEGACY_BIOS_INSTANCE  *Private;
>                           ^~~~~~~
> cc1: all warnings being treated as errors
> 
> Can you please send a patch?
> 
> Thanks!
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel