Topics

[PATCH v2 9/9] UefiCpuPkg/CpuMpPei: Enable paging and set NP flag to avoid TOCTOU (CVE-2019-11098)

Guomin Jiang
 

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

To avoid the TOCTOU, enable paging and set Not Present flag so when
access any code in the flash range, it will trigger #NP exception.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 3 +++
UefiCpuPkg/CpuMpPei/CpuPaging.c | 17 +++++++++++++++--
2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
index caead3ce34d4..fd50b55f06cb 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
@@ -46,6 +46,9 @@ [LibraryClasses]
BaseMemoryLib
CpuLib

+[Guids]
+ gEdkiiMigratedFvInfoGuid ## SOMETIMES_CONSUMES ## HOB
+
[Ppis]
gEfiPeiMpServicesPpiGuid ## PRODUCES
gEfiSecPlatformInformationPpiGuid ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
index d0cbebf70bbf..af4069b42cdb 100644
--- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
+++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
@@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>
#include <Library/CpuLib.h>
#include <Library/BaseLib.h>
+#include <Guid/MigratedFvInfo.h>

#include "CpuMpPei.h"

@@ -605,6 +606,8 @@ MemoryDiscoveredPpiNotifyCallback (
EFI_STATUS Status;
BOOLEAN InitStackGuard;
BOOLEAN InterruptState;
+ EDKII_MIGRATED_FV_INFO *MigratedFvInfo;
+ EFI_PEI_HOB_POINTERS Hob;

InterruptState = SaveAndDisableInterrupts ();
Status = MigrateGdt ();
@@ -617,9 +620,9 @@ MemoryDiscoveredPpiNotifyCallback (
// the task switch (for the sake of stack switch).
//
InitStackGuard = FALSE;
- if (IsIa32PaeSupported () && PcdGetBool (PcdCpuStackGuard)) {
+ if (IsIa32PaeSupported ()) {
EnablePaging ();
- InitStackGuard = TRUE;
+ InitStackGuard = PcdGetBool (PcdCpuStackGuard);
}

Status = InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServices);
@@ -629,6 +632,16 @@ MemoryDiscoveredPpiNotifyCallback (
SetupStackGuardPage ();
}

+ Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid);
+ while (Hob.Raw != NULL) {
+ MigratedFvInfo = GET_GUID_HOB_DATA (Hob);
+ ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase, MigratedFvInfo->FvLength, 0);
+
+ Hob.Raw = GET_NEXT_HOB (Hob);
+ Hob.Raw = GetNextGuidHob (&gEdkiiMigratedFvInfoGuid, Hob.Raw);
+ }
+ CpuFlushTlb ();
+
return Status;
}

--
2.25.1.windows.1

Laszlo Ersek
 

On 07/02/20 07:15, Guomin Jiang wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614

To avoid the TOCTOU, enable paging and set Not Present flag so when
access any code in the flash range, it will trigger #NP exception.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 3 +++
UefiCpuPkg/CpuMpPei/CpuPaging.c | 17 +++++++++++++++--
2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
index caead3ce34d4..fd50b55f06cb 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
@@ -46,6 +46,9 @@ [LibraryClasses]
BaseMemoryLib
CpuLib

+[Guids]
+ gEdkiiMigratedFvInfoGuid ## SOMETIMES_CONSUMES ## HOB
+
[Ppis]
gEfiPeiMpServicesPpiGuid ## PRODUCES
gEfiSecPlatformInformationPpiGuid ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
index d0cbebf70bbf..af4069b42cdb 100644
--- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
+++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
@@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>
#include <Library/CpuLib.h>
#include <Library/BaseLib.h>
+#include <Guid/MigratedFvInfo.h>

#include "CpuMpPei.h"

@@ -605,6 +606,8 @@ MemoryDiscoveredPpiNotifyCallback (
EFI_STATUS Status;
BOOLEAN InitStackGuard;
BOOLEAN InterruptState;
+ EDKII_MIGRATED_FV_INFO *MigratedFvInfo;
+ EFI_PEI_HOB_POINTERS Hob;

InterruptState = SaveAndDisableInterrupts ();
Status = MigrateGdt ();
@@ -617,9 +620,9 @@ MemoryDiscoveredPpiNotifyCallback (
// the task switch (for the sake of stack switch).
//
InitStackGuard = FALSE;
- if (IsIa32PaeSupported () && PcdGetBool (PcdCpuStackGuard)) {
+ if (IsIa32PaeSupported ()) {
EnablePaging ();
- InitStackGuard = TRUE;
+ InitStackGuard = PcdGetBool (PcdCpuStackGuard);
}

Status = InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServices);
@@ -629,6 +632,16 @@ MemoryDiscoveredPpiNotifyCallback (
SetupStackGuardPage ();
}

+ Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid);
+ while (Hob.Raw != NULL) {
+ MigratedFvInfo = GET_GUID_HOB_DATA (Hob);
+ ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase, MigratedFvInfo->FvLength, 0);
+
+ Hob.Raw = GET_NEXT_HOB (Hob);
+ Hob.Raw = GetNextGuidHob (&gEdkiiMigratedFvInfoGuid, Hob.Raw);
+ }
+ CpuFlushTlb ();
+
return Status;
}

(1) This patch calls EnablePaging() even if no
"gEdkiiMigratedFvInfoGuid" HOB exists.

In other words, assume "PcdMigrateTemporaryRamFirmwareVolumes" is FALSE.

- In that case, PeiCore() will not call EvacuateTempRam().

- Consequently, zero "gEdkiiMigratedFvInfoGuid" HOBs are going to be built.

- Consequently, this patch will never call ConvertMemoryPageAttributes()

Why do we call EnablePaging() then (assuming IsIa32PaeSupported()
returns TRUE)?


(2) Consider the opposite case. Assume IsIa32PaeSupported() returns
FALSE. Further assume that at least one "gEdkiiMigratedFvInfoGuid" HOB
exists.

Then ConvertMemoryPageAttributes() will be called, without us ever
calling EnablePaging(). That doesn't seem right.

I suggest:


BOOLEAN InitStackGuard;
EFI_PEI_HOB_POINTERS Hob;

InitStackGuard = FALSE;
Hob.Raw = NULL;

//
// Both the "stack guard" feature and the "temp RAM evacuation"
// feature depend on IA32 PAE support.
//
if (IsIa32PaeSupported ()) {
InitStackGuard = PcdGetBool (PcdCpuStackGuard);
Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid);
}

//
// If either feature is active, then paging is required; enable it.
//
if (InitStackGuard || Hob.Raw != NULL) {
EnablePaging ();
}

/* ... */

if (InitStackGuard) {
SetupStackGuardPage ();
}

/* note: no need to call GetFirstGuidHob() again! */
while (Hob.Raw != NULL) {
/* ... */
}
CpuFlushTlb ();


Thanks
Laszlo