Skip to content

Commit

Permalink
MdeModulePkg and UefiCpuPkg CodeQL Fixes (#561)
Browse files Browse the repository at this point in the history
## Description

CodeQL fixes for MpLib, PeiMpLib and DxeMpLib. This includes their
related files.

- [ ] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

Tested on Intel physical devices. I was able to build and boot without
any problems.

## Integration Instructions

N/A
  • Loading branch information
kenlautner authored Oct 23, 2023
1 parent cf73dc0 commit 1f32c26
Show file tree
Hide file tree
Showing 17 changed files with 306 additions and 23 deletions.
2 changes: 2 additions & 0 deletions MdeModulePkg/MdeModulePkg.dsc
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@

MmuLib|MdePkg/Library/BaseMmuLibNull/BaseMmuLibNull.inf ## MU_CHANGE

PanicLib|MdePkg/Library/BasePanicLibNull/BasePanicLibNull.inf # MU_CHANGE

# MU_CHANGE START Include MemoryProtectionHobLib
[LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.DXE_CORE, LibraryClasses.common.UEFI_APPLICATION]
DxeMemoryProtectionHobLib|MdeModulePkg/Library/MemoryProtectionHobLib/DxeMemoryProtectionHobLib.inf
Expand Down
18 changes: 17 additions & 1 deletion MdeModulePkg/Universal/PCD/Pei/Pcd.c
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,15 @@ PcdSetNvStoreDefaultIdCallBack (
//
NvStoreBuffer = (VARIABLE_STORE_HEADER *)((UINT8 *)DataHeader + sizeof (DataHeader->DataSize) + DataHeader->HeaderSize);
VarStoreHobData = (UINT8 *)BuildGuidHob (&NvStoreBuffer->Signature, NvStoreBuffer->Size);
ASSERT (VarStoreHobData != NULL);
// MU_CHANGE [BEGIN] - CodeQL change
if (VarStoreHobData == NULL) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed build NV Store guid hob.\n", __func__));
ASSERT (VarStoreHobData != NULL);
return;
}

// MU_CHANGE [END] - CodeQL change

CopyMem (VarStoreHobData, NvStoreBuffer, NvStoreBuffer->Size);
//
// Find the matched SkuId and DefaultId in the first section
Expand Down Expand Up @@ -316,6 +324,14 @@ EndOfPeiSignalPpiNotifyCallback (
if (PcdDb != NULL) {
Length = PeiPcdDb->LengthForAllSkus;
Database = BuildGuidHob (&gPcdDataBaseHobGuid, Length);
// MU_CHANGE [BEGIN] - CodeQL change
if (Database == NULL) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed to build PCD guid hob.\n", __func__));
return EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE [END] - CodeQL change

CopyMem (Database, PcdDb, Length);
}

Expand Down
35 changes: 33 additions & 2 deletions MdeModulePkg/Universal/PCD/Pei/Service.c
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ BuildPcdDatabase (
IN EFI_PEI_FILE_HANDLE FileHandle
)
{
VOID *Hob; // MU_CHANGE - CodeQL change
PEI_PCD_DATABASE *Database;
PEI_PCD_DATABASE *PeiPcdDbBinary;
VOID *CallbackFnTable;
Expand All @@ -438,9 +439,31 @@ BuildPcdDatabase (
//
PeiPcdDbBinary = LocateExPcdBinary (FileHandle);

ASSERT (PeiPcdDbBinary != NULL);
// MU_CHANGE [BEGIN] - CodeQL change
if (PeiPcdDbBinary == NULL) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed To locate the Pcd Db binary.\n", __func__));
ASSERT (PeiPcdDbBinary != NULL);
return NULL;
}

Database = BuildGuidHob (&gPcdDataBaseHobGuid, PeiPcdDbBinary->Length + PeiPcdDbBinary->UninitDataBaseSize);
// MU_CHANGE [END] - CodeQL change

// MU_CHANGE [BEGIN] - CodeQL change
// Check to see if the Hob already exists because we can error out of this function when
// creating the CallbackFnTable Hob and call into this function again.
Hob = GetFirstGuidHob (&gPcdDataBaseHobGuid);
if (Hob == NULL) {
Database = BuildGuidHob (&gPcdDataBaseHobGuid, PeiPcdDbBinary->Length + PeiPcdDbBinary->UninitDataBaseSize);
} else {
Database = (PEI_PCD_DATABASE *)GET_GUID_HOB_DATA (Hob);
}

if (Database == NULL) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed to build the PCD Database guid hob.\n", __func__));
return NULL;
}

// MU_CHANGE [END] - CodeQL change

ZeroMem (Database, PeiPcdDbBinary->Length + PeiPcdDbBinary->UninitDataBaseSize);

Expand All @@ -453,6 +476,14 @@ BuildPcdDatabase (

CallbackFnTable = BuildGuidHob (&gEfiCallerIdGuid, SizeOfCallbackFnTable);

// MU_CHANGE [BEGIN] - CodeQL change
if (CallbackFnTable == NULL) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed to build the CallbackFnTable guid hob.\n", __func__));
return NULL;
}

// MU_CHANGE [END] - CodeQL change

ZeroMem (CallbackFnTable, SizeOfCallbackFnTable);

return Database;
Expand Down
11 changes: 11 additions & 0 deletions MdeModulePkg/Universal/ResetSystemPei/ResetSystem.c
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,16 @@ ResetSystem2 (
RecursionDepthPointer = (UINT8 *)GET_GUID_HOB_DATA (Hob);
}

// MU_CHANGE [BEGIN] - CodeQL change
if (RecursionDepthPointer == NULL) {
// Critical build failure so Panicking
PANIC ("Failed to build or get the RecursionDepthPointer Hob");
// Reset anyway if we can't get the RecursionDepthPointer
goto Done;
}

// MU_CHANGE [END] - CodeQL change

//
// Only do REPORT_STATUS_CODE() on first call to ResetSystem()
//
Expand Down Expand Up @@ -338,6 +348,7 @@ ResetSystem2 (
DEBUG ((DEBUG_ERROR, "PEI ResetSystem2: Maximum reset call depth is met. Use the current reset type: %s!\n", mResetTypeStr[ResetType]));
}

Done:
switch (ResetType) {
case EfiResetWarm:
ResetWarm ();
Expand Down
1 change: 1 addition & 0 deletions MdeModulePkg/Universal/ResetSystemPei/ResetSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <Library/HobLib.h>
#include <Library/ResetSystemLib.h>
#include <Library/ReportStatusCodeLib.h>
#include <Library/PanicLib.h> // MU_CHANGE

//
// The maximum recursion depth to ResetSystem() by reset notification handlers
Expand Down
1 change: 1 addition & 0 deletions MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
PeimEntryPoint
HwResetSystemLib ## MS_CHANGE - Use HW reset from reset arch providers.
ReportStatusCodeLib
PanicLib ## MU_CHANGE

[Ppis]
gEfiPeiReset2PpiGuid ## PRODUCES
Expand Down
1 change: 1 addition & 0 deletions UefiCpuPkg/CpuDxe/CpuDxe.inf
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
PeCoffGetEntryPointLib
DxeMemoryProtectionHobLib ## MU_CHANGE
DeviceStateLib ## MU_CHANGE
PanicLib ## MU_CHANGE

[Sources]
CpuDxe.c
Expand Down
11 changes: 10 additions & 1 deletion UefiCpuPkg/CpuDxe/CpuMp.c
Original file line number Diff line number Diff line change
Expand Up @@ -622,8 +622,17 @@ InitializeExceptionStackSwitchHandlers (
{
EXCEPTION_STACK_SWITCH_CONTEXT *SwitchStackData;
UINTN Index;
EFI_STATUS Status; // MU_CHANGE - CodeQL change

MpInitLibWhoAmI (&Index);
// MU_CHANGE [START] - CodeQL change
Status = MpInitLibWhoAmI (&Index);

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed to get processor number. The exception stack was not initialized.\n", __func__));
return;
}

// MU_CHANGE [END] - CodeQL change
SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
//
// This may be called twice for each Cpu. Only run InitializeSeparateExceptionStacks
Expand Down
30 changes: 25 additions & 5 deletions UefiCpuPkg/CpuDxe/CpuPageTable.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <Library/SerialPortLib.h>
#include <Library/SynchronizationLib.h>
#include <Library/PrintLib.h>
#include <Library/PanicLib.h> // MU_CHANGE
#include <Protocol/SmmBase2.h>
#include <Register/Intel/Cpuid.h>
#include <Register/Intel/Msr.h>
Expand Down Expand Up @@ -1258,11 +1259,20 @@ DebugExceptionHandler (
IN EFI_SYSTEM_CONTEXT SystemContext
)
{
UINTN CpuIndex;
UINTN PFEntry;
BOOLEAN IsWpEnabled;
UINTN CpuIndex;
UINTN PFEntry;
BOOLEAN IsWpEnabled;
EFI_STATUS Status; // MU_CHANGE - CodeQL change

MpInitLibWhoAmI (&CpuIndex);
// MU_CHANGE [START] - CodeQL change
Status = MpInitLibWhoAmI (&CpuIndex);

if (EFI_ERROR (Status)) {
PANIC ("Failed to get processor number in the DebugExceptionHandler");
goto Done;
}

// MU_CHANGE [END] - CodeQL change

//
// Clear last PF entries
Expand All @@ -1287,6 +1297,7 @@ DebugExceptionHandler (
//
mPFEntryCount[CpuIndex] = 0;

Done:
//
// Flush TLB
//
Expand Down Expand Up @@ -1337,7 +1348,15 @@ PageFaultExceptionHandler (
}

if (NonStopMode) {
MpInitLibWhoAmI (&CpuIndex);
// MU_CHANGE [START] - CodeQL change
Status = MpInitLibWhoAmI (&CpuIndex);

if (EFI_ERROR (Status)) {
PANIC ("Failed to get processor number in the PageFaultExceptionHandler");
goto Done;
}

// MU_CHANGE [END] - CodeQL change
GetCurrentPagingContext (&PagingContext);
//
// Memory operation cross page boundary, like "rep mov" instruction, will
Expand Down Expand Up @@ -1378,6 +1397,7 @@ PageFaultExceptionHandler (
}
}

Done:
//
// Initialize the serial port before dumping.
//
Expand Down
18 changes: 14 additions & 4 deletions UefiCpuPkg/CpuMpPei/CpuMpPei.c
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,18 @@ InitializeExceptionStackSwitchHandlers (
{
EXCEPTION_STACK_SWITCH_CONTEXT *SwitchStackData;
UINTN Index;
EFI_STATUS Status; // MU_CHANGE - CodeQL change

Status = MpInitLibWhoAmI (&Index);

// MU_CHANGE [BEGIN] - CodeQL change
if (EFI_ERROR (Status)) {
PANIC ("Failed to get processor number when initializing the stack switch exception handlers.");
return;
}

// MU_CHANGE [END] - CodeQL change

MpInitLibWhoAmI (&Index);
SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
//
// This function may be called twice for each Cpu. Only run InitializeSeparateExceptionStacks
Expand Down Expand Up @@ -545,7 +555,7 @@ InitializeMpExceptionStackSwitchHandlers (
Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
if (EFI_ERROR (Status)) {
ASSERT_EFI_ERROR (Status);
DEBUG ((DEBUG_ERROR, "%a - Failed to get number of processors. Status = %r\n", __FUNCTION__, Status));
DEBUG ((DEBUG_ERROR, "%a - Failed to get number of processors. Status = %r\n", __func__, Status));
return;
}

Expand All @@ -555,7 +565,7 @@ InitializeMpExceptionStackSwitchHandlers (
// MU_CHANGE [BEGIN] - CodeQL change
if (SwitchStackData == NULL) {
ASSERT (SwitchStackData != NULL);
DEBUG ((DEBUG_ERROR, "%a - Failed to allocate Switch Stack pages.\n", __FUNCTION__));
DEBUG ((DEBUG_ERROR, "%a - Failed to allocate Switch Stack pages.\n", __func__));
return;
}

Expand Down Expand Up @@ -592,7 +602,7 @@ InitializeMpExceptionStackSwitchHandlers (
// MU_CHANGE [BEGIN] - CodeQL change
if (Buffer == NULL) {
ASSERT (Buffer != NULL);
DEBUG ((DEBUG_ERROR, "%a - Failed to allocate Buffer pages.\n", __FUNCTION__));
DEBUG ((DEBUG_ERROR, "%a - Failed to allocate Buffer pages.\n", __func__));
return;
}

Expand Down
1 change: 1 addition & 0 deletions UefiCpuPkg/CpuMpPei/CpuMpPei.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <Library/MpInitLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PanicLib.h> // MU_CHANGE

extern EFI_PEI_PPI_DESCRIPTOR mPeiCpuMpPpiDesc;

Expand Down
1 change: 1 addition & 0 deletions UefiCpuPkg/CpuMpPei/CpuMpPei.inf
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
BaseMemoryLib
CpuLib
MemoryAllocationLib
PanicLib ## MU_CHANGE

[Guids]
gEdkiiMigratedFvInfoGuid ## SOMETIMES_CONSUMES ## HOB
Expand Down
9 changes: 8 additions & 1 deletion UefiCpuPkg/CpuMpPei/CpuPaging.c
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,14 @@ SetupStackGuardPage (
Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
ASSERT_EFI_ERROR (Status);
if (!EFI_ERROR (Status)) {
MpInitLibWhoAmI (&Bsp);
// MU_CHANGE [BEGIN] - CodeQL change
Status = MpInitLibWhoAmI (&Bsp);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed to get processor number. Aborting Stack Guard Page setup.\n", __func__));
return;
}

// MU_CHANGE [END] - CodeQL change
for (Index = 0; Index < NumberOfProcessors; ++Index) {
StackBase = 0;

Expand Down
11 changes: 10 additions & 1 deletion UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
Original file line number Diff line number Diff line change
Expand Up @@ -611,8 +611,17 @@ RelocateApLoop (
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
UINTN StackStart;
EFI_STATUS Status; // MU_CHANGE - CodeQL change

MpInitLibWhoAmI (&ProcessorNumber);
// MU_CHANGE [START] - CodeQL change
Status = MpInitLibWhoAmI (&ProcessorNumber);

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "[%a] - Failed to get processor number. Aborting AP sync.\n", __func__));
return;
}

// MU_CHANGE [END] - CodeQL change
CpuMpData = GetCpuMpData ();
MwaitSupport = IsMwaitSupport ();
if (CpuMpData->UseSevEsAPMethod) {
Expand Down
Loading

0 comments on commit 1f32c26

Please sign in to comment.