Date   

[PATCH] EmbeddedPkg/GdbStub: Check DebugImageInfoTable type safely

Marvin Häuser
 

C does not allow casting to or dereferencing incompatible pointer
types. Use the ImageInfoType member of the union first to determine
the data type before dereferencing NormalImage.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
EmbeddedPkg/GdbStub/GdbStub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/GdbStub/GdbStub.c b/EmbeddedPkg/GdbStub/GdbStub.c
index 7f2a5ed20011..09167fdafb4d 100644
--- a/EmbeddedPkg/GdbStub/GdbStub.c
+++ b/EmbeddedPkg/GdbStub/GdbStub.c
@@ -1043,8 +1043,8 @@ QxferLibrary (

if (gDebugTable != NULL) {
for (; gEfiDebugImageTableEntry < gDebugImageTableHeader->TableSize; gEfiDebugImageTableEntry++, gDebugTable++) {
- if (gDebugTable->NormalImage != NULL) {
- if ((gDebugTable->NormalImage->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
+ if (gDebugTable->ImageInfoType != NULL) {
+ if ((*gDebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
(gDebugTable->NormalImage->LoadedImageProtocolInstance != NULL)) {
Pdb = PeCoffLoaderGetDebuggerInfo (
gDebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase,
--
2.31.1


[PATCH] SecurityPkg/SecureBootConfigDxe: Fix certificate lookup algorithm

Marvin Häuser
 

The current certificate lookup code does not check the bounds of the
authentication data before accessing it. Abort if the header cannot
fit, and proceed to the next hashing algortihm if the OID of the
current one exceeds the authentication data bounds.

Additionally move the two-byte encoding check out of the loop as the
data is invariant.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 45 ++++++++++++--------
1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index 65a8188d6d03..fd7629f61862 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
@@ -1969,30 +1969,41 @@ HashPeImageByType (
{
UINT8 Index;
WIN_CERTIFICATE_EFI_PKCS *PkcsCertData;
+ UINT32 AuthDataSize;

PkcsCertData = (WIN_CERTIFICATE_EFI_PKCS *) (mImageBase + mSecDataDir->Offset);
+ if (PkcsCertData->Hdr.dwLength <= sizeof (PkcsCertData->Hdr)) {
+ return EFI_UNSUPPORTED;
+ }
+
+ AuthDataSize = PkcsCertData->Hdr.dwLength - sizeof (PkcsCertData->Hdr);
+ if (AuthDataSize < 32) {
+ return EFI_UNSUPPORTED;
+ }
+ //
+ // Check the Hash algorithm in PE/COFF Authenticode.
+ // According to PKCS#7 Definition:
+ // SignedData ::= SEQUENCE {
+ // version Version,
+ // digestAlgorithms DigestAlgorithmIdentifiers,
+ // contentInfo ContentInfo,
+ // .... }
+ // The DigestAlgorithmIdentifiers can be used to determine the hash algorithm in PE/COFF hashing
+ // This field has the fixed offset (+32) in final Authenticode ASN.1 data.
+ // Fixed offset (+32) is calculated based on two bytes of length encoding.
+ //
+ if ((*(PkcsCertData->CertData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) {
+ //
+ // Only support two bytes of Long Form of Length Encoding.
+ //
+ return EFI_UNSUPPORTED;
+ }

for (Index = 0; Index < HASHALG_MAX; Index++) {
- //
- // Check the Hash algorithm in PE/COFF Authenticode.
- // According to PKCS#7 Definition:
- // SignedData ::= SEQUENCE {
- // version Version,
- // digestAlgorithms DigestAlgorithmIdentifiers,
- // contentInfo ContentInfo,
- // .... }
- // The DigestAlgorithmIdentifiers can be used to determine the hash algorithm in PE/COFF hashing
- // This field has the fixed offset (+32) in final Authenticode ASN.1 data.
- // Fixed offset (+32) is calculated based on two bytes of length encoding.
- //
- if ((*(PkcsCertData->CertData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) {
- //
- // Only support two bytes of Long Form of Length Encoding.
- //
+ if (AuthDataSize - 32 < mHash[Index].OidLength) {
continue;
}

- //
if (CompareMem (PkcsCertData->CertData + 32, mHash[Index].OidValue, mHash[Index].OidLength) == 0) {
break;
}
--
2.31.1


[PATCH] UefiCpuPkg/BaseUefiCpuLib: Use toolchain-specific rodata section name

Marvin Häuser
 

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

Correctly define the read-only data sections with the
toolchain-specific section name. This hardens image permission
security and may save image space.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm | 2 +-
UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm b/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
index 5e27cc325012..cfb8bf4a5ae0 100644
--- a/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
+++ b/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
@@ -6,7 +6,7 @@
;*
;------------------------------------------------------------------------------

- SECTION .rodata
+ SECTION RODATA_SECTION_NAME

;
; Float control word initial value:
diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm b/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
index 8485b4713548..3c976a21e391 100644
--- a/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
+++ b/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
@@ -6,7 +6,7 @@
;*
;------------------------------------------------------------------------------

- SECTION .rodata
+ SECTION RODATA_SECTION_NAME
;
; Float control word initial value:
; all exceptions masked, double-extended-precision, round-to-nearest
--
2.31.1


[PATCH] MdeModulePkg/DxeCore: Fix DebugImageInfoTable size report

Marvin Häuser
 

Separate tracking the used entries from the table's self-reported
size. Removing an entry from the table does not necessarily reduce
the size of the table as defragmentation is not performed.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index 7bd970115111..cc22e23eb0b3 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -18,6 +18,8 @@ EFI_DEBUG_IMAGE_INFO_TABLE_HEADER mDebugInfoTableHeader = {

UINTN mMaxTableEntries = 0;

+UINTN mUsedTableEntries = 0;
+
EFI_SYSTEM_TABLE_POINTER *mDebugTable = NULL;

#define EFI_DEBUG_TABLE_ENTRY_SIZE (sizeof (VOID *))
@@ -178,7 +180,7 @@ CoreNewDebugImageInfoEntry (

Table = mDebugInfoTableHeader.EfiDebugImageInfoTable;

- if (mDebugInfoTableHeader.TableSize < mMaxTableEntries) {
+ if (mUsedTableEntries < mMaxTableEntries) {
//
// We still have empty entires in the Table, find the first empty entry.
//
@@ -237,8 +239,17 @@ CoreNewDebugImageInfoEntry (
// increase the number of EFI_DEBUG_IMAGE_INFO elements.
//
mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
+ mUsedTableEntries++;
Table[Index].NormalImage = NormalImage;
- mDebugInfoTableHeader.TableSize++;
+ //
+ // Only increase the amount of elements in the table if the new entry did
+ // not take the place of a previously removed entry.
+ //
+ if (Index == mDebugInfoTableHeader.TableSize) {
+ mDebugInfoTableHeader.TableSize++;
+ }
+
+ ASSERT (Index < mDebugInfoTableHeader.TableSize);
}
mDebugInfoTableHeader.UpdateStatus &= ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
}
@@ -274,9 +285,10 @@ CoreRemoveDebugImageInfoEntry (
mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
Table[Index].NormalImage = NULL;
//
- // Decrease the number of EFI_DEBUG_IMAGE_INFO elements.
+ // Do not reduce the amount of elements reported to be in the table as
+ // this would only work for the last element without defragmentation.
//
- mDebugInfoTableHeader.TableSize--;
+ mUsedTableEntries--;
//
// Free up the record.
//
--
2.31.1


[PATCH] BaseTools/CommonLib: Fix unaligned API prototypes

Marvin Häuser
 

C prohibits not only dereferencing but also casting to unaligned
pointers. Thus, the current set of unaligned APIs cannot be called
safely. Update their prototypes to take VOID * pointers, which must
be able to represent any valid pointer.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
BaseTools/Source/C/Common/CommonLib.c | 16 ++++++++--------
BaseTools/Source/C/Common/CommonLib.h | 8 ++++----
2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
index 7fb4ab764fcd..f1223fb2ae0a 100644
--- a/BaseTools/Source/C/Common/CommonLib.c
+++ b/BaseTools/Source/C/Common/CommonLib.c
@@ -1154,23 +1154,23 @@ StrSize (

UINT64
ReadUnaligned64 (
- CONST UINT64 *Buffer
+ CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);

- return *Buffer;
+ return *(CONST UINT64 *) Buffer;
}

UINT64
WriteUnaligned64 (
- UINT64 *Buffer,
+ VOID *Buffer,
UINT64 Value
)
{
ASSERT (Buffer != NULL);

- return *Buffer = Value;
+ return *(UINT64 *) Buffer = Value;
}


@@ -2018,23 +2018,23 @@ AllocatePool (

UINT16
WriteUnaligned16 (
- UINT16 *Buffer,
+ VOID *Buffer,
UINT16 Value
)
{
ASSERT (Buffer != NULL);

- return *Buffer = Value;
+ return *(UINT16 *) Buffer = Value;
}

UINT16
ReadUnaligned16 (
- CONST UINT16 *Buffer
+ CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);

- return *Buffer;
+ return *(CONST UINT16 *) Buffer;
}
/**
Return whether the integer string is a hex string.
diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
index 0f05d88db206..67c42a91765d 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -238,13 +238,13 @@ CopyGuid (

UINT64
WriteUnaligned64 (
- UINT64 *Buffer,
+ VOID *Buffer,
UINT64 Value
);

UINT64
ReadUnaligned64 (
- CONST UINT64 *Buffer
+ CONST VOID *Buffer
);

UINTN
@@ -363,13 +363,13 @@ AllocatePool (

UINT16
WriteUnaligned16 (
- UINT16 *Buffer,
+ VOID *Buffer,
UINT16 Value
);

UINT16
ReadUnaligned16 (
- CONST UINT16 *Buffer
+ CONST VOID *Buffer
);

VOID *
--
2.31.1


[PATCH] UefiPayloadPkg/UefiPayloadEntry: Fix memory corruption

Marvin Häuser
 

UefiPayloadEntry's AllocatePool() applies the "sizeof" operator to
HOB index rather than the HOB header structure. This yields 4 Bytes
compared to the 8 Bytes the structure header requires. Fix the call
to allocate the required space instead.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c b/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
index 1204573b3e09..f3494969e5ac 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
@@ -163,7 +163,7 @@ AllocatePool (
return NULL;
}

- Hob = (EFI_HOB_MEMORY_POOL *)CreateHob (EFI_HOB_TYPE_MEMORY_POOL, (UINT16)(sizeof (EFI_HOB_TYPE_MEMORY_POOL) + AllocationSize));
+ Hob = (EFI_HOB_MEMORY_POOL *)CreateHob (EFI_HOB_TYPE_MEMORY_POOL, (UINT16)(sizeof (EFI_HOB_MEMORY_POOL) + AllocationSize));
return (VOID *)(Hob + 1);
}

--
2.31.1


[PATCH] StandaloneMmPkg: Support CLANGPDB X64 builds

Marvin Häuser
 

Currently, the flag "-fpie" is passed for all builds with a GCC
family toolchain, including CLANGPDB. CLANGPDB however does not
support this flag as it generates PE/COFF files directly.

As the flag is mostly required for AArch64-specific self-relocation,
drop it for X64 builds and document the limitation to enable X64
CLANGPDB builds of StandaloneMmCore.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
StandaloneMmPkg/Core/StandaloneMmCore.inf | 9 +++++++--
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 7 ++++++-
2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index 87bf6e9440a7..e3349fff29cc 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -76,6 +76,11 @@ [Guids]
gEfiEventExitBootServicesGuid
gEfiEventReadyToBootGuid

+#
+# This configuration fails for CLANGPDB, which does not support PIE in the GCC
+# sense. Such however is required for AArch64 StandaloneMmCore self-relocation,
+# and thus the CLANGPDB toolchain is unsupported for AArch64 for this module.
+#
[BuildOptions]
- GCC:*_*_*_CC_FLAGS = -fpie
- GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
+ GCC:*_*_AARCH64_CC_FLAGS = -fpie
+ GCC:*_*_AARCH64_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58ef4..dcbb082d4ab8 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -54,5 +54,10 @@ [Guids]
[FeaturePcd.AARCH64]
gArmTokenSpaceGuid.PcdFfaEnable

+#
+# This configuration fails for CLANGPDB, which does not support PIE in the GCC
+# sense. Such however is required for AArch64 StandaloneMmCore self-relocation,
+# and thus the CLANGPDB toolchain is unsupported for AArch64 for this module.
+#
[BuildOptions]
- GCC:*_*_*_CC_FLAGS = -fpie
+ GCC:*_*_AARCH64_CC_FLAGS = -fpie
--
2.31.1


[PATCH] StandaloneMmPkg/StandaloneMmCore: Drop unused fixed address feature

Marvin Häuser
 

StandaloneMmCore does not support fixed load addresses for modules.
Remove the unreferenced functions that are used in other dispatchers
to implement this feature.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
StandaloneMmPkg/Core/Dispatcher.c | 167 --------------------
1 file changed, 167 deletions(-)

diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dispatcher.c
index 7e4bf5e94025..d5631f47be68 100644
--- a/StandaloneMmPkg/Core/Dispatcher.c
+++ b/StandaloneMmPkg/Core/Dispatcher.c
@@ -109,173 +109,6 @@ BOOLEAN gRequestDispatch = FALSE;
//
GLOBAL_REMOVE_IF_UNREFERENCED UINT64 *mMmCodeMemoryRangeUsageBitMap=NULL;

-/**
- To check memory usage bit map array to figure out if the memory range in which the image will be loaded
- is available or not. If memory range is avaliable, the function will mark the corresponding bits to 1
- which indicates the memory range is used. The function is only invoked when load modules at fixed address
- feature is enabled.
-
- @param ImageBase The base addres the image will be loaded at.
- @param ImageSize The size of the image
-
- @retval EFI_SUCCESS The memory range the image will be loaded in is available
- @retval EFI_NOT_FOUND The memory range the image will be loaded in is not available
-**/
-EFI_STATUS
-CheckAndMarkFixLoadingMemoryUsageBitMap (
- IN EFI_PHYSICAL_ADDRESS ImageBase,
- IN UINTN ImageSize
- )
-{
- UINT32 MmCodePageNumber;
- UINT64 MmCodeSize;
- EFI_PHYSICAL_ADDRESS MmCodeBase;
- UINTN BaseOffsetPageNumber;
- UINTN TopOffsetPageNumber;
- UINTN Index;
-
- //
- // Build tool will calculate the smm code size and then patch the PcdLoadFixAddressMmCodePageNumber
- //
- MmCodePageNumber = 0;
- MmCodeSize = EFI_PAGES_TO_SIZE (MmCodePageNumber);
- MmCodeBase = gLoadModuleAtFixAddressMmramBase;
-
- //
- // If the memory usage bit map is not initialized, do it. Every bit in the array
- // indicate the status of the corresponding memory page, available or not
- //
- if (mMmCodeMemoryRangeUsageBitMap == NULL) {
- mMmCodeMemoryRangeUsageBitMap = AllocateZeroPool (((MmCodePageNumber / 64) + 1) * sizeof (UINT64));
- }
-
- //
- // If the Dxe code memory range is not allocated or the bit map array allocation failed, return EFI_NOT_FOUND
- //
- if (mMmCodeMemoryRangeUsageBitMap == NULL) {
- return EFI_NOT_FOUND;
- }
-
- //
- // see if the memory range for loading the image is in the MM code range.
- //
- if (MmCodeBase + MmCodeSize < ImageBase + ImageSize || MmCodeBase > ImageBase) {
- return EFI_NOT_FOUND;
- }
-
- //
- // Test if the memory is available or not.
- //
- BaseOffsetPageNumber = (UINTN)EFI_SIZE_TO_PAGES ((UINT32)(ImageBase - MmCodeBase));
- TopOffsetPageNumber = (UINTN)EFI_SIZE_TO_PAGES ((UINT32)(ImageBase + ImageSize - MmCodeBase));
- for (Index = BaseOffsetPageNumber; Index < TopOffsetPageNumber; Index ++) {
- if ((mMmCodeMemoryRangeUsageBitMap[Index / 64] & LShiftU64 (1, (Index % 64))) != 0) {
- //
- // This page is already used.
- //
- return EFI_NOT_FOUND;
- }
- }
-
- //
- // Being here means the memory range is available. So mark the bits for the memory range
- //
- for (Index = BaseOffsetPageNumber; Index < TopOffsetPageNumber; Index ++) {
- mMmCodeMemoryRangeUsageBitMap[Index / 64] |= LShiftU64 (1, (Index % 64));
- }
- return EFI_SUCCESS;
-}
-
-/**
- Get the fixed loading address from image header assigned by build tool. This function only be called
- when Loading module at Fixed address feature enabled.
-
- @param ImageContext Pointer to the image context structure that describes the PE/COFF
- image that needs to be examined by this function.
- @retval EFI_SUCCESS An fixed loading address is assigned to this image by build tools .
- @retval EFI_NOT_FOUND The image has no assigned fixed loadding address.
-
-**/
-EFI_STATUS
-GetPeCoffImageFixLoadingAssignedAddress(
- IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
- )
-{
- UINTN SectionHeaderOffset;
- EFI_STATUS Status;
- EFI_IMAGE_SECTION_HEADER SectionHeader;
- EFI_IMAGE_OPTIONAL_HEADER_UNION *ImgHdr;
- EFI_PHYSICAL_ADDRESS FixLoadingAddress;
- UINT16 Index;
- UINTN Size;
- UINT16 NumberOfSections;
- UINT64 ValueInSectionHeader;
-
- FixLoadingAddress = 0;
- Status = EFI_NOT_FOUND;
-
- //
- // Get PeHeader pointer
- //
- ImgHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((CHAR8* )ImageContext->Handle + ImageContext->PeCoffHeaderOffset);
- SectionHeaderOffset = ImageContext->PeCoffHeaderOffset + sizeof (UINT32) + sizeof (EFI_IMAGE_FILE_HEADER) +
- ImgHdr->Pe32.FileHeader.SizeOfOptionalHeader;
- NumberOfSections = ImgHdr->Pe32.FileHeader.NumberOfSections;
-
- //
- // Get base address from the first section header that doesn't point to code section.
- //
- for (Index = 0; Index < NumberOfSections; Index++) {
- //
- // Read section header from file
- //
- Size = sizeof (EFI_IMAGE_SECTION_HEADER);
- Status = ImageContext->ImageRead (
- ImageContext->Handle,
- SectionHeaderOffset,
- &Size,
- &SectionHeader
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = EFI_NOT_FOUND;
-
- if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_CNT_CODE) == 0) {
- //
- // Build tool will save the address in PointerToRelocations & PointerToLineNumbers fields
- // in the first section header that doesn't point to code section in image header. So there
- // is an assumption that when the feature is enabled, if a module with a loading address
- // assigned by tools, the PointerToRelocations & PointerToLineNumbers fields should not be
- // Zero, or else, these 2 fields should be set to Zero
- //
- ValueInSectionHeader = ReadUnaligned64 ((UINT64*)&SectionHeader.PointerToRelocations);
- if (ValueInSectionHeader != 0) {
- //
- // Found first section header that doesn't point to code section in which build tool saves the
- // offset to SMRAM base as image base in PointerToRelocations & PointerToLineNumbers fields
- //
- FixLoadingAddress = (EFI_PHYSICAL_ADDRESS)(gLoadModuleAtFixAddressMmramBase + (INT64)ValueInSectionHeader);
- //
- // Check if the memory range is available.
- //
- Status = CheckAndMarkFixLoadingMemoryUsageBitMap (FixLoadingAddress, (UINTN)(ImageContext->ImageSize + ImageContext->SectionAlignment));
- if (!EFI_ERROR(Status)) {
- //
- // The assigned address is valid. Return the specified loading address
- //
- ImageContext->ImageAddress = FixLoadingAddress;
- }
- }
- break;
- }
- SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER);
- }
- DEBUG ((DEBUG_INFO|DEBUG_LOAD, "LOADING MODULE FIXED INFO: Loading module at fixed address %x, Status = %r\n",
- FixLoadingAddress, Status));
- return Status;
-}
/**
Loads an EFI image into SMRAM.

--
2.31.1


[PATCH] StandaloneMmPkg/StandaloneMmCore: Drop code for traditional drivers

Marvin Häuser
 

StandaloneMmCore has code paths in place to support traditional MM
drivers based on the availability of mEfiSystemTable. This variable
is not populated anywhere however, rendering said paths unreachable.
Remove the unreachable support code.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
StandaloneMmPkg/Core/Dependency.c | 6 ---
StandaloneMmPkg/Core/Dispatcher.c | 51 +-------------------
StandaloneMmPkg/Core/StandaloneMmCore.c | 1 -
StandaloneMmPkg/Core/StandaloneMmCore.h | 1 -
4 files changed, 2 insertions(+), 57 deletions(-)

diff --git a/StandaloneMmPkg/Core/Dependency.c b/StandaloneMmPkg/Core/Dependency.c
index eb4baa4086f0..3ae0201eb647 100644
--- a/StandaloneMmPkg/Core/Dependency.c
+++ b/StandaloneMmPkg/Core/Dependency.c
@@ -242,12 +242,6 @@ MmIsSchedulable (
CopyMem (&DriverGuid, Iterator + 1, sizeof (EFI_GUID));

Status = MmLocateProtocol (&DriverGuid, NULL, &Interface);
- if (EFI_ERROR (Status) && (mEfiSystemTable != NULL)) {
- //
- // For MM Driver, it may depend on uefi protocols
- //
- Status = mEfiSystemTable->BootServices->LocateProtocol (&DriverGuid, NULL, &Interface);
- }

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_DISPATCH, " PUSH GUID(%g) = FALSE\n", &DriverGuid));
diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dispatcher.c
index 7e4bf5e94025..76d33f81fef1 100644
--- a/StandaloneMmPkg/Core/Dispatcher.c
+++ b/StandaloneMmPkg/Core/Dispatcher.c
@@ -364,45 +364,6 @@ MmLoadImage (
DriverEntry->ImageBuffer = DstBuffer;
DriverEntry->NumberOfPage = PageCount;

- if (mEfiSystemTable != NULL) {
- Status = mEfiSystemTable->BootServices->AllocatePool (
- EfiBootServicesData,
- sizeof (EFI_LOADED_IMAGE_PROTOCOL),
- (VOID **)&DriverEntry->LoadedImage
- );
- if (EFI_ERROR (Status)) {
- MmFreePages (DstBuffer, PageCount);
- return Status;
- }
-
- ZeroMem (DriverEntry->LoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL));
- //
- // Fill in the remaining fields of the Loaded Image Protocol instance.
- // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed.
- //
- DriverEntry->LoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
- DriverEntry->LoadedImage->ParentHandle = NULL;
- DriverEntry->LoadedImage->SystemTable = mEfiSystemTable;
- DriverEntry->LoadedImage->DeviceHandle = NULL;
- DriverEntry->LoadedImage->FilePath = NULL;
-
- DriverEntry->LoadedImage->ImageBase = (VOID *)(UINTN)DriverEntry->ImageBuffer;
- DriverEntry->LoadedImage->ImageSize = ImageContext.ImageSize;
- DriverEntry->LoadedImage->ImageCodeType = EfiRuntimeServicesCode;
- DriverEntry->LoadedImage->ImageDataType = EfiRuntimeServicesData;
-
- //
- // Create a new image handle in the UEFI handle database for the MM Driver
- //
- DriverEntry->ImageHandle = NULL;
- Status = mEfiSystemTable->BootServices->InstallMultipleProtocolInterfaces (
- &DriverEntry->ImageHandle,
- &gEfiLoadedImageProtocolGuid,
- DriverEntry->LoadedImage,
- NULL
- );
- }
-
//
// Print the load address and the PDB file name if it is available
//
@@ -637,16 +598,8 @@ MmDispatcher (
//
// For each MM driver, pass NULL as ImageHandle
//
- if (mEfiSystemTable == NULL) {
- DEBUG ((DEBUG_INFO, "StartImage - 0x%x (Standalone Mode)\n", DriverEntry->ImageEntryPoint));
- Status = ((MM_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint) (DriverEntry->ImageHandle, &gMmCoreMmst);
- } else {
- DEBUG ((DEBUG_INFO, "StartImage - 0x%x (Tradition Mode)\n", DriverEntry->ImageEntryPoint));
- Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint) (
- DriverEntry->ImageHandle,
- mEfiSystemTable
- );
- }
+ DEBUG ((DEBUG_INFO, "StartImage - 0x%x (Standalone Mode)\n", DriverEntry->ImageEntryPoint));
+ Status = ((MM_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint) (DriverEntry->ImageHandle, &gMmCoreMmst);
if (EFI_ERROR(Status)) {
DEBUG ((DEBUG_INFO, "StartImage Status - %r\n", Status));
MmFreePages(DriverEntry->ImageBuffer, DriverEntry->NumberOfPage);
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Core/StandaloneMmCore.c
index fbb0ec75e557..45976f203dd9 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.c
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
@@ -92,7 +92,6 @@ MM_CORE_MMI_HANDLERS mMmCoreMmiHandlers[] = {
{ NULL, NULL, NULL, FALSE },
};

-EFI_SYSTEM_TABLE *mEfiSystemTable;
UINTN mMmramRangeCount;
EFI_MMRAM_DESCRIPTOR *mMmramRanges;

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h b/StandaloneMmPkg/Core/StandaloneMmCore.h
index 2a89edd0fc46..f6b3cc861e39 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.h
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
@@ -848,6 +848,5 @@ DumpMmramInfo (

extern UINTN mMmramRangeCount;
extern EFI_MMRAM_DESCRIPTOR *mMmramRanges;
-extern EFI_SYSTEM_TABLE *mEfiSystemTable;

#endif
--
2.31.1


[PATCH] StandaloneMmPkg/FvLib: Correct FV section data size

Marvin Häuser
 

The size of a FV section includes the size of its header. Subtract
latter to yield the correct size of the contained data.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
StandaloneMmPkg/Library/FvLib/FvLib.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/StandaloneMmPkg/Library/FvLib/FvLib.c b/StandaloneMmPkg/Library/FvLib/FvLib.c
index 94139ae3898b..5d5ad8f73fe0 100644
--- a/StandaloneMmPkg/Library/FvLib/FvLib.c
+++ b/StandaloneMmPkg/Library/FvLib/FvLib.c
@@ -359,16 +359,22 @@ FfsFindSectionData (
ParsedLength = 0;
while (ParsedLength < FileSize) {
if (Section->Type == SectionType) {
+ //
+ // Size is 24 bits wide so mask upper 8 bits.
+ //
+ SectionLength = SECTION_SIZE (Section);
+
+ if (SectionLength < sizeof (*Section)) {
+ return EFI_VOLUME_CORRUPTED;
+ }
*SectionData = (VOID *) (Section + 1);
- *SectionDataSize = SECTION_SIZE(Section);
+ *SectionDataSize = SectionLength - sizeof (*Section);
return EFI_SUCCESS;
}
//
- // Size is 24 bits wide so mask upper 8 bits.
// SectionLength is adjusted it is 4 byte aligned.
// Go to the next section
//
- SectionLength = SECTION_SIZE(Section);
SectionLength = GET_OCCUPIED_SIZE (SectionLength, 4);

ParsedLength += SectionLength;
--
2.31.1


[PATCH] SecurityPkg/DxeImageVerificationLib: Fix certificate lookup algorithm

Marvin Häuser
 

The current certificate lookup code does not check the bounds of the
authentication data before accessing it. Abort if the header cannot
fit. Also, the lookup code aborts once the authetication data is
smaller than an algorithm's OID size. As OIDs are variably-sized,
this may cause unexpected authentication failure due to the early
error-exit.

Additionally move the two-byte encoding check out of the loop as the
data is invariant.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 43 +++++++++++---------
1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index c48861cd6496..6615099baafb 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -624,30 +624,33 @@ HashPeImageByType (
{
UINT8 Index;

+ if (AuthDataSize < 32) {
+ return EFI_UNSUPPORTED;
+ }
+ //
+ // Check the Hash algorithm in PE/COFF Authenticode.
+ // According to PKCS#7 Definition:
+ // SignedData ::= SEQUENCE {
+ // version Version,
+ // digestAlgorithms DigestAlgorithmIdentifiers,
+ // contentInfo ContentInfo,
+ // .... }
+ // The DigestAlgorithmIdentifiers can be used to determine the hash algorithm in PE/COFF hashing
+ // This field has the fixed offset (+32) in final Authenticode ASN.1 data.
+ // Fixed offset (+32) is calculated based on two bytes of length encoding.
+ //
+ if ((*(AuthData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) {
+ //
+ // Only support two bytes of Long Form of Length Encoding.
+ //
+ return EFI_UNSUPPORTED;
+ }
+
for (Index = 0; Index < HASHALG_MAX; Index++) {
- //
- // Check the Hash algorithm in PE/COFF Authenticode.
- // According to PKCS#7 Definition:
- // SignedData ::= SEQUENCE {
- // version Version,
- // digestAlgorithms DigestAlgorithmIdentifiers,
- // contentInfo ContentInfo,
- // .... }
- // The DigestAlgorithmIdentifiers can be used to determine the hash algorithm in PE/COFF hashing
- // This field has the fixed offset (+32) in final Authenticode ASN.1 data.
- // Fixed offset (+32) is calculated based on two bytes of length encoding.
- //
- if ((*(AuthData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) {
- //
- // Only support two bytes of Long Form of Length Encoding.
- //
+ if (AuthDataSize - 32 < mHash[Index].OidLength) {
continue;
}

- if (AuthDataSize < 32 + mHash[Index].OidLength) {
- return EFI_UNSUPPORTED;
- }
-
if (CompareMem (AuthData + 32, mHash[Index].OidValue, mHash[Index].OidLength) == 0) {
break;
}
--
2.31.1


[PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256 hash in dbx

Marvin Häuser
 

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

The UEFI specification prohibits loading any UEFI image of which a
matching SHA-256 hash is contained in "dbx" (UEFI 2.9, 32.5.3.3
"Authorization Process", 3.A). Currently, this is only explicitly
checked when the image is unsigned and otherwise the hash algorithms
of the certificates are used.

Align with the UEFI specification by specifically looking up the
SHA-256 hash of the image in "dbx".

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 60 ++++++++------------
1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index c48861cd6496..1f9bb33e86c3 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1803,34 +1803,36 @@ DxeImageVerificationHandler (
}
}

+ //
+ // The SHA256 hash value of the image must not be reflected in the security data base "dbx".
+ //
+ if (!HashPeImage (HASHALG_SHA256)) {
+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image using %s.\n", mHashTypeStr));
+ goto Failed;
+ }
+
+ DbStatus = IsSignatureFoundInDatabase (
+ EFI_IMAGE_SECURITY_DATABASE1,
+ mImageDigest,
+ &mCertType,
+ mImageDigestSize,
+ &IsFound
+ );
+ if (EFI_ERROR (DbStatus) || IsFound) {
+ //
+ // Image Hash is in forbidden database (DBX).
+ //
+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
+ goto Failed;
+ }
+
//
// Start Image Validation.
//
if (SecDataDir == NULL || SecDataDir->Size == 0) {
//
- // This image is not signed. The SHA256 hash value of the image must match a record in the security database "db",
- // and not be reflected in the security data base "dbx".
+ // This image is not signed. The SHA256 hash value of the image must match a record in the security database "db".
//
- if (!HashPeImage (HASHALG_SHA256)) {
- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image using %s.\n", mHashTypeStr));
- goto Failed;
- }
-
- DbStatus = IsSignatureFoundInDatabase (
- EFI_IMAGE_SECURITY_DATABASE1,
- mImageDigest,
- &mCertType,
- mImageDigestSize,
- &IsFound
- );
- if (EFI_ERROR (DbStatus) || IsFound) {
- //
- // Image Hash is in forbidden database (DBX).
- //
- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
- goto Failed;
- }
-
DbStatus = IsSignatureFoundInDatabase (
EFI_IMAGE_SECURITY_DATABASE,
mImageDigest,
@@ -1932,20 +1934,6 @@ DxeImageVerificationHandler (
//
// Check the image's hash value.
//
- DbStatus = IsSignatureFoundInDatabase (
- EFI_IMAGE_SECURITY_DATABASE1,
- mImageDigest,
- &mCertType,
- mImageDigestSize,
- &IsFound
- );
- if (EFI_ERROR (DbStatus) || IsFound) {
- Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s hash of image is found in DBX.\n", mHashTypeStr));
- IsVerified = FALSE;
- break;
- }
-
if (!IsVerified) {
DbStatus = IsSignatureFoundInDatabase (
EFI_IMAGE_SECURITY_DATABASE,
--
2.31.1


[PATCH] MdePkg/BaseLib: Fix unaligned API prototypes

Marvin Häuser
 

C prohibits not only dereferencing but also casting to unaligned
pointers. Thus, the current set of unaligned APIs cannot be called
safely. Update their prototypes to take VOID * pointers, which must
be able to represent any valid pointer.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdePkg/Library/BaseLib/Arm/Unaligned.c | 14 ++++-----
MdePkg/Library/BaseLib/Unaligned.c | 32 ++++++++++----------
MdePkg/Include/Library/BaseLib.h | 16 +++++-----
3 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/MdePkg/Library/BaseLib/Arm/Unaligned.c b/MdePkg/Library/BaseLib/Arm/Unaligned.c
index e9934e7003cb..57f19fc44e0b 100644
--- a/MdePkg/Library/BaseLib/Arm/Unaligned.c
+++ b/MdePkg/Library/BaseLib/Arm/Unaligned.c
@@ -59,7 +59,7 @@ ReadUnaligned16 (
UINT16
EFIAPI
WriteUnaligned16 (
- OUT UINT16 *Buffer,
+ OUT VOID *Buffer,
IN UINT16 Value
)
{
@@ -87,7 +87,7 @@ WriteUnaligned16 (
UINT32
EFIAPI
ReadUnaligned24 (
- IN CONST UINT32 *Buffer
+ IN CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);
@@ -116,7 +116,7 @@ ReadUnaligned24 (
UINT32
EFIAPI
WriteUnaligned24 (
- OUT UINT32 *Buffer,
+ OUT VOID *Buffer,
IN UINT32 Value
)
{
@@ -143,7 +143,7 @@ WriteUnaligned24 (
UINT32
EFIAPI
ReadUnaligned32 (
- IN CONST UINT32 *Buffer
+ IN CONST VOID *Buffer
)
{
UINT16 LowerBytes;
@@ -175,7 +175,7 @@ ReadUnaligned32 (
UINT32
EFIAPI
WriteUnaligned32 (
- OUT UINT32 *Buffer,
+ OUT VOID *Buffer,
IN UINT32 Value
)
{
@@ -202,7 +202,7 @@ WriteUnaligned32 (
UINT64
EFIAPI
ReadUnaligned64 (
- IN CONST UINT64 *Buffer
+ IN CONST VOID *Buffer
)
{
UINT32 LowerBytes;
@@ -234,7 +234,7 @@ ReadUnaligned64 (
UINT64
EFIAPI
WriteUnaligned64 (
- OUT UINT64 *Buffer,
+ OUT VOID *Buffer,
IN UINT64 Value
)
{
diff --git a/MdePkg/Library/BaseLib/Unaligned.c b/MdePkg/Library/BaseLib/Unaligned.c
index a419cb85e53c..3041adcde606 100644
--- a/MdePkg/Library/BaseLib/Unaligned.c
+++ b/MdePkg/Library/BaseLib/Unaligned.c
@@ -26,12 +26,12 @@
UINT16
EFIAPI
ReadUnaligned16 (
- IN CONST UINT16 *Buffer
+ IN CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);

- return *Buffer;
+ return *(CONST UINT16 *) Buffer;
}

/**
@@ -52,13 +52,13 @@ ReadUnaligned16 (
UINT16
EFIAPI
WriteUnaligned16 (
- OUT UINT16 *Buffer,
+ OUT VOID *Buffer,
IN UINT16 Value
)
{
ASSERT (Buffer != NULL);

- return *Buffer = Value;
+ return *(UINT16 *) Buffer = Value;
}

/**
@@ -77,12 +77,12 @@ WriteUnaligned16 (
UINT32
EFIAPI
ReadUnaligned24 (
- IN CONST UINT32 *Buffer
+ IN CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);

- return *Buffer & 0xffffff;
+ return *(CONST UINT32 *) Buffer & 0xffffff;
}

/**
@@ -103,13 +103,13 @@ ReadUnaligned24 (
UINT32
EFIAPI
WriteUnaligned24 (
- OUT UINT32 *Buffer,
+ OUT VOID *Buffer,
IN UINT32 Value
)
{
ASSERT (Buffer != NULL);

- *Buffer = BitFieldWrite32 (*Buffer, 0, 23, Value);
+ *(UINT32 *) Buffer = BitFieldWrite32 (*(CONST UINT32 *) Buffer, 0, 23, Value);
return Value;
}

@@ -129,12 +129,12 @@ WriteUnaligned24 (
UINT32
EFIAPI
ReadUnaligned32 (
- IN CONST UINT32 *Buffer
+ IN CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);

- return *Buffer;
+ return *(CONST UINT32 *) Buffer;
}

/**
@@ -155,13 +155,13 @@ ReadUnaligned32 (
UINT32
EFIAPI
WriteUnaligned32 (
- OUT UINT32 *Buffer,
+ OUT VOID *Buffer,
IN UINT32 Value
)
{
ASSERT (Buffer != NULL);

- return *Buffer = Value;
+ return *(UINT32 *) Buffer = Value;
}

/**
@@ -180,12 +180,12 @@ WriteUnaligned32 (
UINT64
EFIAPI
ReadUnaligned64 (
- IN CONST UINT64 *Buffer
+ IN CONST VOID *Buffer
)
{
ASSERT (Buffer != NULL);

- return *Buffer;
+ return *(CONST UINT64 *) Buffer;
}

/**
@@ -206,11 +206,11 @@ ReadUnaligned64 (
UINT64
EFIAPI
WriteUnaligned64 (
- OUT UINT64 *Buffer,
+ OUT VOID *Buffer,
IN UINT64 Value
)
{
ASSERT (Buffer != NULL);

- return *Buffer = Value;
+ return *(UINT64 *) Buffer = Value;
}
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 2452c1d92e51..4d30f0539c6b 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -3420,7 +3420,7 @@ DivS64x64Remainder (
UINT16
EFIAPI
ReadUnaligned16 (
- IN CONST UINT16 *Buffer
+ IN CONST VOID *Buffer
);


@@ -3442,7 +3442,7 @@ ReadUnaligned16 (
UINT16
EFIAPI
WriteUnaligned16 (
- OUT UINT16 *Buffer,
+ OUT VOID *Buffer,
IN UINT16 Value
);

@@ -3463,7 +3463,7 @@ WriteUnaligned16 (
UINT32
EFIAPI
ReadUnaligned24 (
- IN CONST UINT32 *Buffer
+ IN CONST VOID *Buffer
);


@@ -3485,7 +3485,7 @@ ReadUnaligned24 (
UINT32
EFIAPI
WriteUnaligned24 (
- OUT UINT32 *Buffer,
+ OUT VOID *Buffer,
IN UINT32 Value
);

@@ -3506,7 +3506,7 @@ WriteUnaligned24 (
UINT32
EFIAPI
ReadUnaligned32 (
- IN CONST UINT32 *Buffer
+ IN CONST VOID *Buffer
);


@@ -3528,7 +3528,7 @@ ReadUnaligned32 (
UINT32
EFIAPI
WriteUnaligned32 (
- OUT UINT32 *Buffer,
+ OUT VOID *Buffer,
IN UINT32 Value
);

@@ -3549,7 +3549,7 @@ WriteUnaligned32 (
UINT64
EFIAPI
ReadUnaligned64 (
- IN CONST UINT64 *Buffer
+ IN CONST VOID *Buffer
);


@@ -3571,7 +3571,7 @@ ReadUnaligned64 (
UINT64
EFIAPI
WriteUnaligned64 (
- OUT UINT64 *Buffer,
+ OUT VOID *Buffer,
IN UINT64 Value
);

--
2.31.1


[PATCH] MdePkg/Base.h: Introduce various alignment-related macros

Marvin Häuser
 

ALIGNOF: Determining the alignment requirement of data types is
crucial to ensure safe memory accesses when parsing untrusted data.

IS_POW2: Determining whether a value is a power of two is important
to verify whether untrusted values are valid alignment values.

IS_ALIGNED: In combination with ALIGNOF data offsets can be verified.
A more general version of IS_ALIGNED defined by several modules.

ADDRESS_IS_ALIGNED: Variant of IS_ALIGNED for pointers and addresses.
Replaces module-specific definitions throughout the codebase.

ALIGN_VALUE_ADDEND: The added to align up can be used to directly
determine the required offset for data alignment.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c | 2 +-
MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c | 6 +-
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 12 +--
MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c | 2 +-
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 4 +-
MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c | 6 +-
MdeModulePkg/Universal/EbcDxe/EbcExecute.c | 36 ++++----
MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h | 1 -
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 2 -
MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.h | 1 -
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h | 4 +-
MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.h | 2 -
MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h | 2 -
MdeModulePkg/Universal/EbcDxe/EbcExecute.h | 3 +-
MdePkg/Include/Base.h | 90 +++++++++++++++++++-
15 files changed, 125 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c b/MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c
index 7636ad27c86c..520197aee752 100644
--- a/MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c
@@ -2099,7 +2099,7 @@ TrustTransferAtaDevice (
// ATA PassThru PPI.
//
if ((AtaPassThru->Mode->IoAlign > 1) &&
- !IS_ALIGNED (Buffer, AtaPassThru->Mode->IoAlign)) {
+ !ADDRESS_IS_ALIGNED (Buffer, AtaPassThru->Mode->IoAlign)) {
NewBuffer = AllocateAlignedPages (
EFI_SIZE_TO_PAGES (TransferLength),
AtaPassThru->Mode->IoAlign
diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c b/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
index 191b78c88541..057ad42d596b 100644
--- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
@@ -193,15 +193,15 @@ AhciAtaPassThruPassThru (
}

IoAlign = This->Mode->IoAlign;
- if ((IoAlign > 1) && !IS_ALIGNED (Packet->InDataBuffer, IoAlign)) {
+ if ((IoAlign > 1) && !ADDRESS_IS_ALIGNED (Packet->InDataBuffer, IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((IoAlign > 1) && !IS_ALIGNED (Packet->OutDataBuffer, IoAlign)) {
+ if ((IoAlign > 1) && !ADDRESS_IS_ALIGNED (Packet->OutDataBuffer, IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((IoAlign > 1) && !IS_ALIGNED (Packet->Asb, IoAlign)) {
+ if ((IoAlign > 1) && !ADDRESS_IS_ALIGNED (Packet->Asb, IoAlign)) {
return EFI_INVALID_PARAMETER;
}

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 86fe9d954fdb..c7b3cfce1340 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1281,15 +1281,15 @@ AtaPassThruPassThru (

Instance = ATA_PASS_THRU_PRIVATE_DATA_FROM_THIS (This);

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->InDataBuffer, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->InDataBuffer, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->OutDataBuffer, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->OutDataBuffer, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->Asb, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->Asb, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

@@ -2012,15 +2012,15 @@ ExtScsiPassThruPassThru (
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->InDataBuffer, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->InDataBuffer, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->OutDataBuffer, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->OutDataBuffer, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->SenseData, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->SenseData, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

diff --git a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
index 79026a4a957d..eabab8ac5bc5 100644
--- a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
+++ b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
@@ -1036,7 +1036,7 @@ TrustTransferAtaDevice (
// Check the alignment of the incoming buffer prior to invoking underlying ATA PassThru
//
AtaPassThru = AtaDevice->AtaBusDriverData->AtaPassThru;
- if ((AtaPassThru->Mode->IoAlign > 1) && !IS_ALIGNED (Buffer, AtaPassThru->Mode->IoAlign)) {
+ if ((AtaPassThru->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED (Buffer, AtaPassThru->Mode->IoAlign)) {
NewBuffer = AllocateAlignedBuffer (AtaDevice, TransferLength);
if (NewBuffer == NULL) {
return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
index c80e78fa8a6b..81db2efd0599 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
+++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
@@ -1956,7 +1956,7 @@ ScsiDiskReceiveData (
goto Done;
}

- if ((ScsiDiskDevice->ScsiIo->IoAlign > 1) && !IS_ALIGNED (PayloadBuffer, ScsiDiskDevice->ScsiIo->IoAlign)) {
+ if ((ScsiDiskDevice->ScsiIo->IoAlign > 1) && !ADDRESS_IS_ALIGNED (PayloadBuffer, ScsiDiskDevice->ScsiIo->IoAlign)) {
AlignedBuffer = AllocateAlignedBuffer (ScsiDiskDevice, PayloadBufferSize);
if (AlignedBuffer == NULL) {
Status = EFI_OUT_OF_RESOURCES;
@@ -2171,7 +2171,7 @@ ScsiDiskSendData (
goto Done;
}

- if ((ScsiDiskDevice->ScsiIo->IoAlign > 1) && !IS_ALIGNED (PayloadBuffer, ScsiDiskDevice->ScsiIo->IoAlign)) {
+ if ((ScsiDiskDevice->ScsiIo->IoAlign > 1) && !ADDRESS_IS_ALIGNED (PayloadBuffer, ScsiDiskDevice->ScsiIo->IoAlign)) {
AlignedBuffer = AllocateAlignedBuffer (ScsiDiskDevice, PayloadBufferSize);
if (AlignedBuffer == NULL) {
Status = EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
index 92ff958f161e..c4d01a20fcbe 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
@@ -170,15 +170,15 @@ UfsPassThruPassThru (
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->InDataBuffer, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->InDataBuffer, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->OutDataBuffer, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->OutDataBuffer, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

- if ((This->Mode->IoAlign > 1) && !IS_ALIGNED(Packet->SenseData, This->Mode->IoAlign)) {
+ if ((This->Mode->IoAlign > 1) && !ADDRESS_IS_ALIGNED(Packet->SenseData, This->Mode->IoAlign)) {
return EFI_INVALID_PARAMETER;
}

diff --git a/MdeModulePkg/Universal/EbcDxe/EbcExecute.c b/MdeModulePkg/Universal/EbcDxe/EbcExecute.c
index 1c4a4f5155c9..ba66f441bcea 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcExecute.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcExecute.c
@@ -2004,7 +2004,7 @@ ExecuteJMP (
// check for alignment, and jump absolute.
//
Data64 = (UINT64) VmReadImmed64 (VmPtr, 2);
- if (!IS_ALIGNED ((UINTN) Data64, sizeof (UINT16))) {
+ if (!ADDRESS_IS_ALIGNED ((UINTN) Data64, sizeof (UINT16))) {
EbcDebugSignalException (
EXCEPT_EBC_ALIGNMENT_CHECK,
EXCEPTION_FLAG_FATAL,
@@ -2059,7 +2059,7 @@ ExecuteJMP (
// Form: JMP32 @Rx {Index32}
//
Addr = VmReadMemN (VmPtr, (UINTN) Data64 + Index32);
- if (!IS_ALIGNED ((UINTN) Addr, sizeof (UINT16))) {
+ if (!ADDRESS_IS_ALIGNED ((UINTN) Addr, sizeof (UINT16))) {
EbcDebugSignalException (
EXCEPT_EBC_ALIGNMENT_CHECK,
EXCEPTION_FLAG_FATAL,
@@ -2082,7 +2082,7 @@ ExecuteJMP (
// Form: JMP32 Rx {Immed32}
//
Addr = (UINTN) (Data64 + Index32);
- if (!IS_ALIGNED ((UINTN) Addr, sizeof (UINT16))) {
+ if (!ADDRESS_IS_ALIGNED ((UINTN) Addr, sizeof (UINT16))) {
EbcDebugSignalException (
EXCEPT_EBC_ALIGNMENT_CHECK,
EXCEPTION_FLAG_FATAL,
@@ -3128,7 +3128,7 @@ ExecuteRET (
// Pull the return address off the VM app's stack and set the IP
// to it
//
- if (!IS_ALIGNED ((UINTN) VmPtr->Gpr[0], sizeof (UINT16))) {
+ if (!ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Gpr[0], sizeof (UINT16))) {
EbcDebugSignalException (
EXCEPT_EBC_ALIGNMENT_CHECK,
EXCEPTION_FLAG_FATAL,
@@ -4693,7 +4693,7 @@ VmWriteMem16 (
//
// Do a simple write if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINT16))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINT16))) {
*(UINT16 *) Addr = Data;
} else {
//
@@ -4756,7 +4756,7 @@ VmWriteMem32 (
//
// Do a simple write if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINT32))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINT32))) {
*(UINT32 *) Addr = Data;
} else {
//
@@ -4819,7 +4819,7 @@ VmWriteMem64 (
//
// Do a simple write if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINT64))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINT64))) {
*(UINT64 *) Addr = Data;
} else {
//
@@ -4885,7 +4885,7 @@ VmWriteMemN (
//
// Do a simple write if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINTN))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINTN))) {
*(UINTN *) Addr = Data;
} else {
for (Index = 0; Index < sizeof (UINTN) / sizeof (UINT32); Index++) {
@@ -4949,7 +4949,7 @@ VmReadImmed16 (
//
// Read direct if aligned
//
- if (IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (INT16))) {
+ if (ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (INT16))) {
return * (INT16 *) (VmPtr->Ip + Offset);
} else {
//
@@ -4993,7 +4993,7 @@ VmReadImmed32 (
//
// Read direct if aligned
//
- if (IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT32))) {
+ if (ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT32))) {
return * (INT32 *) (VmPtr->Ip + Offset);
}
//
@@ -5032,7 +5032,7 @@ VmReadImmed64 (
//
// Read direct if aligned
//
- if (IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT64))) {
+ if (ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT64))) {
return * (UINT64 *) (VmPtr->Ip + Offset);
}
//
@@ -5069,7 +5069,7 @@ VmReadCode16 (
//
// Read direct if aligned
//
- if (IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT16))) {
+ if (ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT16))) {
return * (UINT16 *) (VmPtr->Ip + Offset);
} else {
//
@@ -5110,7 +5110,7 @@ VmReadCode32 (
//
// Read direct if aligned
//
- if (IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT32))) {
+ if (ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT32))) {
return * (UINT32 *) (VmPtr->Ip + Offset);
}
//
@@ -5147,7 +5147,7 @@ VmReadCode64 (
//
// Read direct if aligned
//
- if (IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT64))) {
+ if (ADDRESS_IS_ALIGNED ((UINTN) VmPtr->Ip + Offset, sizeof (UINT64))) {
return * (UINT64 *) (VmPtr->Ip + Offset);
}
//
@@ -5210,7 +5210,7 @@ VmReadMem16 (
//
// Read direct if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINT16))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINT16))) {
return * (UINT16 *) Addr;
}
//
@@ -5243,7 +5243,7 @@ VmReadMem32 (
//
// Read direct if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINT32))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINT32))) {
return * (UINT32 *) Addr;
}
//
@@ -5280,7 +5280,7 @@ VmReadMem64 (
//
// Read direct if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINT64))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINT64))) {
return * (UINT64 *) Addr;
}
//
@@ -5349,7 +5349,7 @@ VmReadMemN (
//
// Read direct if aligned
//
- if (IS_ALIGNED (Addr, sizeof (UINTN))) {
+ if (ADDRESS_IS_ALIGNED (Addr, sizeof (UINTN))) {
return * (UINTN *) Addr;
}
//
diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h
index 2be78076bee7..59bb9e5d0bca 100644
--- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h
+++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h
@@ -145,7 +145,6 @@ typedef union {
#define AHCI_PORT_SERR 0x0030
#define AHCI_PORT_CI 0x0038

-#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)
#define TIMER_PERIOD_SECONDS(Seconds) MultU64x32((UINT64)(Seconds), 10000000)

#pragma pack(1)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
index 5f582b9b3e76..99bbf7d14a17 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
@@ -147,8 +147,6 @@ struct _ATA_NONBLOCK_TASK {
#define ATA_ATAPI_TIMEOUT EFI_TIMER_PERIOD_SECONDS(3)
#define ATA_SPINUP_TIMEOUT EFI_TIMER_PERIOD_SECONDS(10)

-#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)
-
#define ATA_PASS_THRU_PRIVATE_DATA_FROM_THIS(a) \
CR (a, \
ATA_ATAPI_PASS_THRU_INSTANCE, \
diff --git a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.h b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.h
index a5a865209942..172d2d61ea6c 100644
--- a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.h
+++ b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.h
@@ -76,7 +76,6 @@
#define ATA_TASK_SIGNATURE SIGNATURE_32 ('A', 'T', 'S', 'K')
#define ATA_DEVICE_SIGNATURE SIGNATURE_32 ('A', 'B', 'I', 'D')
#define ATA_SUB_TASK_SIGNATURE SIGNATURE_32 ('A', 'S', 'T', 'S')
-#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)

//
// ATA bus data structure for ATA controller
diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
index ed9bbd6f8ba8..86ad27b3292f 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
+++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
@@ -37,9 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <IndustryStandard/Scsi.h>
#include <IndustryStandard/Atapi.h>

-#define IS_DEVICE_FIXED(a) (a)->FixedDevice ? 1 : 0
-
-#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)
+#define IS_DEVICE_FIXED(a) (a)->FixedDevice ? 1 : 0

#define UFS_WLUN_RPMB 0xC4

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.h b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.h
index 6e2305aa2bc2..7306106a4454 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.h
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.h
@@ -133,8 +133,6 @@ typedef struct _UFS_PEIM_HC_PRIVATE_DATA {

#define ROUNDUP8(x) (((x) % 8 == 0) ? (x) : ((x) / 8 + 1) * 8)

-#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)
-
#define GET_UFS_PEIM_HC_PRIVATE_DATA_FROM_THIS(a) CR (a, UFS_PEIM_HC_PRIVATE_DATA, BlkIoPpi, UFS_PEIM_HC_SIG)
#define GET_UFS_PEIM_HC_PRIVATE_DATA_FROM_THIS2(a) CR (a, UFS_PEIM_HC_PRIVATE_DATA, BlkIo2Ppi, UFS_PEIM_HC_SIG)
#define GET_UFS_PEIM_HC_PRIVATE_DATA_FROM_THIS_NOTIFY(a) CR (a, UFS_PEIM_HC_PRIVATE_DATA, EndOfPeiNotifyList, UFS_PEIM_HC_SIG)
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
index 79b86f7e6b3d..11b5b197b67a 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
@@ -105,8 +105,6 @@ typedef struct {

#define ROUNDUP8(x) (((x) % 8 == 0) ? (x) : ((x) / 8 + 1) * 8)

-#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)
-
#define UFS_PASS_THRU_PRIVATE_DATA_FROM_THIS(a) \
CR (a, \
UFS_PASS_THRU_PRIVATE_DATA, \
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcExecute.h b/MdeModulePkg/Universal/EbcDxe/EbcExecute.h
index 1cb68bc5385a..858ca7fc86ae 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcExecute.h
+++ b/MdeModulePkg/Universal/EbcDxe/EbcExecute.h
@@ -14,8 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
//
// Macros to check and set alignment
//
-#define ASSERT_ALIGNED(addr, size) ASSERT (!((UINT32) (addr) & (size - 1)))
-#define IS_ALIGNED(addr, size) !((UINT32) (addr) & (size - 1))
+#define ASSERT_ALIGNED(addr, size) ASSERT (IS_ALIGNED (addr, size))

//
// Debug macro
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 2da08b0c787f..32d0e512e05f 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -789,6 +789,35 @@ typedef UINTN *BASE_LIST;
#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
#endif

+/**
+ Returns the alignment requirement of a type.
+
+ @param TYPE The name of the type to retrieve the alignment requirement of.
+
+ @return Alignment requirement, in Bytes, of TYPE.
+**/
+#if defined(__GNUC__) || defined(__clang__) || (defined(_MSC_VER) && _MSC_VER >= 1900)
+ //
+ // All supported versions of GCC and Clang, as well as MSVC 2015 and later,
+ // support the standard operator _Alignof.
+ //
+ #define ALIGNOF(TYPE) _Alignof (TYPE)
+#elif defined(_MSC_VER)
+ //
+ // Earlier versions of MSVC, at least MSVC 2008 and later, support the
+ // vendor-extension __alignof.
+ //
+ #define ALIGNOF(TYPE) __alignof (TYPE)
+#else
+ //
+ // For compilers that do not support inbuilt alignof operators, use OFFSET_OF.
+ // CHAR8 is known to have both a size and an alignment requirement of 1 Byte.
+ // As such, A must be located exactly at the offset equal to its alignment
+ // requirement.
+ //
+ #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)
+#endif
+
/**
Portable definition for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.
@@ -824,6 +853,21 @@ STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specif
STATIC_ASSERT (sizeof (L'A') == 2, "sizeof (L'A') does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (L"A") == 4, "sizeof (L\"A\") does not meet UEFI Specification Data Type requirements");

+STATIC_ASSERT (ALIGNOF (BOOLEAN) == sizeof (BOOLEAN), "Alignment of BOOLEAN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT8) == sizeof (INT8), "Alignment of INT8 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT8) == sizeof (UINT8), "Alignment of INT16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT16) == sizeof (INT16), "Alignment of INT16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT16) == sizeof (UINT16), "Alignment of UINT16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT32) == sizeof (INT32), "Alignment of INT32 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT32) == sizeof (UINT32), "Alignment of UINT32 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT64) == sizeof (INT64), "Alignment of INT64 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT64) == sizeof (UINT64), "Alignment of UINT64 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (CHAR8) == sizeof (CHAR8), "Alignment of CHAR8 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (CHAR16) == sizeof (CHAR16), "Alignment of CHAR16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INTN) == sizeof (INTN), "Alignment of INTN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINTN) == sizeof (UINTN), "Alignment of UINTN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (VOID *) == sizeof (VOID *), "Alignment of VOID * does not meet UEFI Specification Data Type requirements");
+
//
// The following three enum types are used to verify that the compiler
// configuration for enum types is compliant with Section 2.3.1 of the
@@ -847,6 +891,10 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not me
STATIC_ASSERT (sizeof (__VERIFY_UINT16_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements");

+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT8_ENUM_SIZE) == sizeof (__VERIFY_UINT8_ENUM_SIZE), "Alignment of enum does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT16_ENUM_SIZE) == sizeof (__VERIFY_UINT16_ENUM_SIZE), "Alignment of enum does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT32_ENUM_SIZE) == sizeof (__VERIFY_UINT32_ENUM_SIZE), "Alignment of enum does not meet UEFI Specification Data Type requirements");
+
/**
Macro that returns a pointer to the data structure that contains a specified field of
that data structure. This is a lightweight method to hide information by placing a
@@ -868,6 +916,46 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
**/
#define BASE_CR(Record, TYPE, Field) ((TYPE *) ((CHAR8 *) (Record) - OFFSET_OF (TYPE, Field)))

+/**
+ Checks whether a value is a power of two.
+
+ @param Value The value to check.
+
+ @return Whether Value is a power of two.
+**/
+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) == 0U)
+
+/**
+ Checks whether a value is aligned by a specified alignment.
+
+ @param Value The value to check.
+ @param Alignment The alignment boundary used to check against.
+
+ @return Whether Value is aligned by Alignment.
+**/
+#define IS_ALIGNED(Value, Alignment) (((Value) & ((Alignment) - 1U)) == 0U)
+
+/**
+ Checks whether a pointer or address is aligned by a specified alignment.
+
+ @param Address The pointer or address to check.
+ @param Alignment The alignment boundary used to check against.
+
+ @return Whether Address is aligned by Alignment.
+**/
+#define ADDRESS_IS_ALIGNED(Address, Alignment) IS_ALIGNED ((UINTN) (Address), Alignment)
+
+/**
+ Determines the addend to add to a value to round it up to the next boundary of
+ a specified alignment.
+
+ @param Value The value to round up.
+ @param Alignment The alignment boundary used to return the addend.
+
+ @return Addend to round Value up to alignment boundary Alignment.
+**/
+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value)) & ((Alignment) - 1U))
+
/**
Rounds a value up to the next boundary using a specified alignment.

@@ -880,7 +968,7 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
@return A value up to the next boundary.

**/
-#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1)))
+#define ALIGN_VALUE(Value, Alignment) ((Value) + ALIGN_VALUE_ADDEND (Value, Alignment))

/**
Adjust a pointer by adding the minimum offset required for it to be aligned on
--
2.31.1


[PATCH] MdeModulePkg/PiSmmIpl: Correct fixed load address bounds check

Marvin Häuser
 

The current code only checks whether PiSmmCore's fixed loading
address, but not its entire memory range, is in bounds of the
reserved area. Furthermore, it does not consider the module's fixed
loading address, which is relative to the reserved area, could
wraparound when added to the base address.

Fix both issues by performing sufficient bounds checks in a way that
is free from wraparounds.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 599a0cd01d80..259cd0bb8924 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -979,7 +979,8 @@ GetPeCoffImageFixLoadingAssignedAddress(
//
FixLoadingAddress = (EFI_PHYSICAL_ADDRESS)(SmramBase + (INT64)ValueInSectionHeader);

- if (SmramBase + SmmCodeSize > FixLoadingAddress && SmramBase <= FixLoadingAddress) {
+ if (ValueInSectionHeader < SmmCodeSize
+ && (UINTN)(ImageContext->ImageSize + ImageContext->SectionAlignment) <= SmmCodeSize - ValueInSectionHeader) {
//
// The assigned address is valid. Return the specified loading address
//
--
2.31.1


[PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands

Marvin Häuser
 

The legacy codebase allowed SMM images to be registered for profiling
from DXE. Support for this has been dropped entirely, so remove the
remaining handlers.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 80 --------------------
MdeModulePkg/Include/Guid/MemoryProfile.h | 5 --
2 files changed, 85 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index 1b302c810cc9..7316df7531fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -2232,64 +2232,6 @@ Done:
mSmramProfileGettingStatus = SmramProfileGettingStatus;
}

-/**
- SMRAM profile handler to register SMM image.
-
- @param SmramProfileParameterRegisterImage The parameter of SMM profile register image.
-
-**/
-VOID
-SmramProfileHandlerRegisterImage (
- IN SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *SmramProfileParameterRegisterImage
- )
-{
- EFI_STATUS Status;
- EFI_SMM_DRIVER_ENTRY DriverEntry;
- VOID *EntryPointInImage;
-
- ZeroMem (&DriverEntry, sizeof (DriverEntry));
- CopyMem (&DriverEntry.FileName, &SmramProfileParameterRegisterImage->FileName, sizeof(EFI_GUID));
- DriverEntry.ImageBuffer = SmramProfileParameterRegisterImage->ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterRegisterImage->NumberOfPage;
- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN) DriverEntry.ImageBuffer, &EntryPointInImage);
- ASSERT_EFI_ERROR (Status);
- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN) EntryPointInImage;
-
- Status = RegisterSmramProfileImage (&DriverEntry, FALSE);
- if (!EFI_ERROR (Status)) {
- SmramProfileParameterRegisterImage->Header.ReturnStatus = 0;
- }
-}
-
-/**
- SMRAM profile handler to unregister SMM image.
-
- @param SmramProfileParameterUnregisterImage The parameter of SMM profile unregister image.
-
-**/
-VOID
-SmramProfileHandlerUnregisterImage (
- IN SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *SmramProfileParameterUnregisterImage
- )
-{
- EFI_STATUS Status;
- EFI_SMM_DRIVER_ENTRY DriverEntry;
- VOID *EntryPointInImage;
-
- ZeroMem (&DriverEntry, sizeof (DriverEntry));
- CopyMem (&DriverEntry.FileName, &SmramProfileParameterUnregisterImage->FileName, sizeof (EFI_GUID));
- DriverEntry.ImageBuffer = SmramProfileParameterUnregisterImage->ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterUnregisterImage->NumberOfPage;
- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN) DriverEntry.ImageBuffer, &EntryPointInImage);
- ASSERT_EFI_ERROR (Status);
- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN) EntryPointInImage;
-
- Status = UnregisterSmramProfileImage (&DriverEntry, FALSE);
- if (!EFI_ERROR (Status)) {
- SmramProfileParameterUnregisterImage->Header.ReturnStatus = 0;
- }
-}
-
/**
Dispatch function for a Software SMI handler.

@@ -2374,28 +2316,6 @@ SmramProfileHandler (
}
SmramProfileHandlerGetDataByOffset ((SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET *) (UINTN) CommBuffer);
break;
- case SMRAM_PROFILE_COMMAND_REGISTER_IMAGE:
- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerRegisterImage\n"));
- if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE)) {
- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer size invalid!\n"));
- return EFI_SUCCESS;
- }
- if (mSmramReadyToLock) {
- return EFI_SUCCESS;
- }
- SmramProfileHandlerRegisterImage ((SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *) (UINTN) CommBuffer);
- break;
- case SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE:
- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerUnregisterImage\n"));
- if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE)) {
- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer size invalid!\n"));
- return EFI_SUCCESS;
- }
- if (mSmramReadyToLock) {
- return EFI_SUCCESS;
- }
- SmramProfileHandlerUnregisterImage ((SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *) (UINTN) CommBuffer);
- break;
case SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE:
DEBUG ((EFI_D_ERROR, "SmramProfileHandlerGetRecordingState\n"));
if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)) {
diff --git a/MdeModulePkg/Include/Guid/MemoryProfile.h b/MdeModulePkg/Include/Guid/MemoryProfile.h
index eee3b9125240..92cd1e7cf493 100644
--- a/MdeModulePkg/Include/Guid/MemoryProfile.h
+++ b/MdeModulePkg/Include/Guid/MemoryProfile.h
@@ -388,11 +388,6 @@ struct _EDKII_MEMORY_PROFILE_PROTOCOL {
//
#define SMRAM_PROFILE_COMMAND_GET_PROFILE_INFO 0x1
#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA 0x2
-//
-// Below 2 commands are now used by ECP only and only valid before SmmReadyToLock
-//
-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3
-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA_BY_OFFSET 0x5
#define SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE 0x6
--
2.31.1


[PATCH] MdeModulePkg/DxeCore: Use the correct source for fixed load address

Marvin Häuser
 

The build tools write the fixed load address into the section header
of the first non-code section. While PeiCore and PiSmmCore correctly
load it from there, DxeCore uses ImageBase from the PE/COFF Optional
Header instead.

Align the behaviour of DxeCore with the other dispatchers.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/Dxe/Image/Image.c | 30 ++++++++++++++------
1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 641a5715b112..9455c2fa45ad 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -416,6 +416,7 @@ GetPeCoffImageFixLoadingAssignedAddress(
EFI_STATUS Status;
EFI_IMAGE_SECTION_HEADER SectionHeader;
EFI_IMAGE_OPTIONAL_HEADER_UNION *ImgHdr;
+ EFI_PHYSICAL_ADDRESS FixLoadingAddress;
UINT16 Index;
UINTN Size;
UINT16 NumberOfSections;
@@ -468,24 +469,35 @@ GetPeCoffImageFixLoadingAssignedAddress(
//
ValueInSectionHeader = ReadUnaligned64((UINT64*)&SectionHeader.PointerToRelocations);
if (ValueInSectionHeader != 0) {
- //
- // When the feature is configured as load module at fixed absolute address, the ImageAddress field of ImageContext
- // hold the specified address. If the feature is configured as load module at fixed offset, ImageAddress hold an offset
- // relative to top address
- //
- if ((INT64)PcdGet64(PcdLoadModuleAtFixAddressEnable) < 0) {
- ImageContext->ImageAddress = gLoadModuleAtFixAddressConfigurationTable.DxeCodeTopAddress + (INT64)(INTN)ImageContext->ImageAddress;
+ if ((INT64)PcdGet64(PcdLoadModuleAtFixAddressEnable) > 0) {
+ //
+ // When LMFA feature is configured as Load Module at Fixed Absolute Address mode, PointerToRelocations & PointerToLineNumbers field
+ // hold the absolute address of image base running in memory
+ //
+ FixLoadingAddress = ValueInSectionHeader;
+ } else {
+ //
+ // When LMFA feature is configured as Load Module at Fixed offset mode, PointerToRelocations & PointerToLineNumbers field
+ // hold the offset relative to a platform-specific top address.
+ //
+ FixLoadingAddress = gLoadModuleAtFixAddressConfigurationTable.DxeCodeTopAddress + ValueInSectionHeader;
}
//
// Check if the memory range is available.
//
- Status = CheckAndMarkFixLoadingMemoryUsageBitMap (ImageContext->ImageAddress, (UINTN)(ImageContext->ImageSize + ImageContext->SectionAlignment));
+ Status = CheckAndMarkFixLoadingMemoryUsageBitMap (FixLoadingAddress, (UINTN)(ImageContext->ImageSize + ImageContext->SectionAlignment));
+ if (!EFI_ERROR(Status)) {
+ //
+ // The assigned address is valid. Return the specified loading address
+ //
+ ImageContext->ImageAddress = FixLoadingAddress;
+ }
}
break;
}
SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER);
}
- DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED INFO: Loading module at fixed address 0x%11p. Status = %r \n", (VOID *)(UINTN)(ImageContext->ImageAddress), Status));
+ DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED INFO: Loading module at fixed address 0x%11p. Status = %r \n", (VOID *)(UINTN)FixLoadingAddress, Status));
return Status;
}

--
2.31.1


[PATCH] MdeModulePkg/DxeCore: Drop unnecessary pointer indirection

Marvin Häuser
 

CoreInitializeGcdServices() takes a pointer-to-pointer for the first
HOB. However, it is dereferenced in every operation inside. To
mitigate confusion whether or not it can be re-allocated by the
callee, remove the unnecessary indirection.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 2 +-
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 10 +++++-----
MdeModulePkg/Core/Dxe/DxeMain.h | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index db21311f9352..1a8f6b57f356 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -263,7 +263,7 @@ DxeMain (
//
// Initialize Memory Services
//
- CoreInitializeMemoryServices (&HobStart, &MemoryBaseAddress, &MemoryLength);
+ CoreInitializeMemoryServices (HobStart, &MemoryBaseAddress, &MemoryLength);

MemoryProfileInit (HobStart);

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 51b082b7e7eb..af9e9e315819 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2168,7 +2168,7 @@ FindLargestFreeRegion (
**/
EFI_STATUS
CoreInitializeMemoryServices (
- IN VOID **HobStart,
+ IN VOID *HobStart,
OUT EFI_PHYSICAL_ADDRESS *MemoryBaseAddress,
OUT UINT64 *MemoryLength
)
@@ -2194,7 +2194,7 @@ CoreInitializeMemoryServices (
//
// Point at the first HOB. This must be the PHIT HOB.
//
- Hob.Raw = *HobStart;
+ Hob.Raw = HobStart;
ASSERT (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_HANDOFF);

//
@@ -2248,7 +2248,7 @@ CoreInitializeMemoryServices (
// Find the Resource Descriptor HOB that contains PHIT range EfiFreeMemoryBottom..EfiFreeMemoryTop
//
Found = FALSE;
- for (Hob.Raw = *HobStart; !END_OF_HOB_LIST(Hob); Hob.Raw = GET_NEXT_HOB(Hob)) {
+ for (Hob.Raw = HobStart; !END_OF_HOB_LIST(Hob); Hob.Raw = GET_NEXT_HOB(Hob)) {
//
// Skip all HOBs except Resource Descriptor HOBs
//
@@ -2304,7 +2304,7 @@ CoreInitializeMemoryServices (
// Compute range between the start of the Resource Descriptor HOB and the start of the HOB List
//
BaseAddress = PageAlignAddress (ResourceHob->PhysicalStart);
- Length = PageAlignLength ((UINT64)((UINTN)*HobStart - BaseAddress));
+ Length = PageAlignLength ((UINT64)((UINTN)HobStart - BaseAddress));
FindLargestFreeRegion (&BaseAddress, &Length, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION));
}
}
@@ -2329,7 +2329,7 @@ CoreInitializeMemoryServices (
// The max address must be within the physically addressible range for the processor.
//
HighAddress = MAX_ALLOC_ADDRESS;
- for (Hob.Raw = *HobStart; !END_OF_HOB_LIST(Hob); Hob.Raw = GET_NEXT_HOB(Hob)) {
+ for (Hob.Raw = HobStart; !END_OF_HOB_LIST(Hob); Hob.Raw = GET_NEXT_HOB(Hob)) {
//
// Skip the Resource Descriptor HOB that contains the PHIT
//
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 9bd3c0d08411..8f268dd2854a 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -366,7 +366,7 @@ CoreAcquireGcdMemoryLock (
**/
EFI_STATUS
CoreInitializeMemoryServices (
- IN VOID **HobStart,
+ IN VOID *HobStart,
OUT EFI_PHYSICAL_ADDRESS *MemoryBaseAddress,
OUT UINT64 *MemoryLength
);
--
2.31.1


[PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Marvin Häuser
 

In theory, modifications to the DebugImageInfoTable may cause
exceptions. If the exception handler parses the table, this can lead
to subsequent exceptions if the table state is inconsistent.

Ensure the DebugImageInfoTable remains consistent during
modifications. This includes:
1) Free the old table only only after the new table has been
published. Mitigates use-after-free of the old table.
2) Do not insert an image entry till it is fully initialised. Entries
may be inserted in the live range if an entry was deleted previously.
Mitigaes the usage of inconsistent entries.
3) Free the old image entry only after the table has been updated
with the NULL value. Mitigates use-after-free of the old entry.
4) Set the MODIFIED state before performing any modifications.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 60 +++++++++++---------
1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index a75d4158280b..7bd970115111 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -165,10 +165,11 @@ CoreNewDebugImageInfoEntry (
IN EFI_HANDLE ImageHandle
)
{
- EFI_DEBUG_IMAGE_INFO *Table;
- EFI_DEBUG_IMAGE_INFO *NewTable;
- UINTN Index;
- UINTN TableSize;
+ EFI_DEBUG_IMAGE_INFO *Table;
+ EFI_DEBUG_IMAGE_INFO *NewTable;
+ UINTN Index;
+ UINTN TableSize;
+ EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;

//
// Set the flag indicating that we're in the process of updating the table.
@@ -203,14 +204,6 @@ CoreNewDebugImageInfoEntry (
// Copy the old table into the new one
//
CopyMem (NewTable, Table, TableSize);
- //
- // Free the old table
- //
- CoreFreePool (Table);
- //
- // Update the table header
- //
- Table = NewTable;
mDebugInfoTableHeader.EfiDebugImageInfoTable = NewTable;
//
// Enlarge the max table entries and set the first empty entry index to
@@ -218,24 +211,34 @@ CoreNewDebugImageInfoEntry (
//
Index = mMaxTableEntries;
mMaxTableEntries += EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;
+ //
+ // Free the old table
+ //
+ CoreFreePool (Table);
+ //
+ // Update the table header
+ //
+ Table = NewTable;
}

//
// Allocate data for new entry
//
- Table[Index].NormalImage = AllocateZeroPool (sizeof (EFI_DEBUG_IMAGE_INFO_NORMAL));
- if (Table[Index].NormalImage != NULL) {
+ NormalImage = AllocateZeroPool (sizeof (EFI_DEBUG_IMAGE_INFO_NORMAL));
+ if (NormalImage != NULL) {
//
// Update the entry
//
- Table[Index].NormalImage->ImageInfoType = (UINT32) ImageInfoType;
- Table[Index].NormalImage->LoadedImageProtocolInstance = LoadedImage;
- Table[Index].NormalImage->ImageHandle = ImageHandle;
+ NormalImage->ImageInfoType = (UINT32) ImageInfoType;
+ NormalImage->LoadedImageProtocolInstance = LoadedImage;
+ NormalImage->ImageHandle = ImageHandle;
//
- // Increase the number of EFI_DEBUG_IMAGE_INFO elements and set the mDebugInfoTable in modified status.
+ // Set the mDebugInfoTable in modified status, insert the entry, and
+ // increase the number of EFI_DEBUG_IMAGE_INFO elements.
//
- mDebugInfoTableHeader.TableSize++;
mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
+ Table[Index].NormalImage = NormalImage;
+ mDebugInfoTableHeader.TableSize++;
}
mDebugInfoTableHeader.UpdateStatus &= ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
}
@@ -253,8 +256,9 @@ CoreRemoveDebugImageInfoEntry (
EFI_HANDLE ImageHandle
)
{
- EFI_DEBUG_IMAGE_INFO *Table;
- UINTN Index;
+ EFI_DEBUG_IMAGE_INFO *Table;
+ UINTN Index;
+ EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;

mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;

@@ -263,16 +267,20 @@ CoreRemoveDebugImageInfoEntry (
for (Index = 0; Index < mMaxTableEntries; Index++) {
if (Table[Index].NormalImage != NULL && Table[Index].NormalImage->ImageHandle == ImageHandle) {
//
- // Found a match. Free up the record, then NULL the pointer to indicate the slot
- // is free.
+ // Found a match. Set the mDebugInfoTable in modified status and NULL the
+ // pointer to indicate the slot is free and.
//
- CoreFreePool (Table[Index].NormalImage);
+ NormalImage = Table[Index].NormalImage;
+ mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
Table[Index].NormalImage = NULL;
//
- // Decrease the number of EFI_DEBUG_IMAGE_INFO elements and set the mDebugInfoTable in modified status.
+ // Decrease the number of EFI_DEBUG_IMAGE_INFO elements.
//
mDebugInfoTableHeader.TableSize--;
- mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
+ //
+ // Free up the record.
+ //
+ CoreFreePool (NormalImage);
break;
}
}
--
2.31.1


[PATCH] EmulatorPkg/Host/Unix: Remove unused declarations

Marvin Häuser
 

Remove declarations of functions that are not implemented at all,
or are implemented elsewhere.

Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
EmulatorPkg/Unix/Host/Host.h | 87 --------------------
1 file changed, 87 deletions(-)

diff --git a/EmulatorPkg/Unix/Host/Host.h b/EmulatorPkg/Unix/Host/Host.h
index 9791cf8c370e..b2e5eafb6323 100644
--- a/EmulatorPkg/Unix/Host/Host.h
+++ b/EmulatorPkg/Unix/Host/Host.h
@@ -145,15 +145,6 @@ typedef struct {
} IMAGE_CONTEXT_TO_MOD_HANDLE;


-EFI_STATUS
-EFIAPI
-SecUnixPeiLoadFile (
- VOID *Pe32Data,
- EFI_PHYSICAL_ADDRESS *ImageAddress,
- UINT64 *ImageSize,
- EFI_PHYSICAL_ADDRESS *EntryPoint
- );
-
int
main (
IN int Argc,
@@ -169,48 +160,6 @@ SecLoadFromCore (
IN VOID *PeiCoreFile
);

-EFI_STATUS
-SecLoadFile (
- IN VOID *Pe32Data,
- IN EFI_PHYSICAL_ADDRESS *ImageAddress,
- IN UINT64 *ImageSize,
- IN EFI_PHYSICAL_ADDRESS *EntryPoint
- );
-
-EFI_STATUS
-SecFfsFindPeiCore (
- IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader,
- OUT VOID **Pe32Data
- );
-
-EFI_STATUS
-SecFfsFindNextFile (
- IN EFI_FV_FILETYPE SearchType,
- IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader,
- IN OUT EFI_FFS_FILE_HEADER **FileHeader
- );
-
-EFI_STATUS
-SecFfsFindSectionData (
- IN EFI_SECTION_TYPE SectionType,
- IN EFI_FFS_FILE_HEADER *FfsFileHeader,
- IN OUT VOID **SectionData
- );
-
-EFI_STATUS
-EFIAPI
-SecUnixPeCoffLoaderLoadAsDll (
- IN CHAR8 *PdbFileName,
- IN VOID **ImageEntryPoint,
- OUT VOID **ModHandle
- );
-
-EFI_STATUS
-EFIAPI
-SecUnixPeCoffLoaderFreeLibrary (
- OUT VOID *ModHandle
- );
-
EFI_STATUS
SecUnixFdAddress (
IN UINTN Index,
@@ -231,12 +180,6 @@ GasketSecUnixFdAddress (
;


-EFI_STATUS
-GetImageReadFunction (
- IN PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
- IN EFI_PHYSICAL_ADDRESS *TopOfMemory
- );
-
EFI_STATUS
EFIAPI
SecImageRead (
@@ -246,36 +189,12 @@ SecImageRead (
OUT VOID *Buffer
);

-CHAR16 *
-AsciiToUnicode (
- IN CHAR8 *Ascii,
- IN UINTN *StrLen OPTIONAL
- );
-
UINTN
CountSeparatorsInString (
IN const CHAR16 *String,
IN CHAR16 Separator
);

-EFI_STATUS
-EFIAPI
-SecTemporaryRamSupport (
- IN CONST EFI_PEI_SERVICES **PeiServices,
- IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
- IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
- IN UINTN CopySize
- );
-
-EFI_STATUS
-EFIAPI
-GasketSecTemporaryRamSupport (
- IN CONST EFI_PEI_SERVICES **PeiServices,
- IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
- IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
- IN UINTN CopySize
- );
-

RETURN_STATUS
EFIAPI
@@ -290,12 +209,6 @@ SecPeCoffRelocateImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
);

-VOID
-EFIAPI
-SecPeCoffLoaderUnloadImageExtraAction (
- IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
- );
-

VOID
PeiSwitchStacks (
--
2.31.1

5441 - 5460 of 84265