From 06c7662c5e35cde03f72a6a804326fbe7ef32c7b Mon Sep 17 00:00:00 2001 From: Chris Fernald Date: Mon, 12 Feb 2024 11:10:40 -0800 Subject: [PATCH] Remove AdvancedLogger MmCoreArm dependence on global variables. (#437) ## Description The Advanced logger Arm MM Core library may be invoked before the imagine has completed it's PeCOFF section attribute fixup. Previously this was handled by explicitly mapping the data page need for the adv logger globals to be writable, but this leads to potential infinite recursion if any of the functions in the MMU stack calls a print. Instead this change is to remove the dependence on global variables and so remove the need for external calls in these library functions. - [X] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Tested and validated presence of MM logs in serial output and adv logger buffer. ## Integration Instructions N/A --- .../MmCoreArm/AdvancedLoggerLib.c | 130 ++++++------------ .../MmCoreArm/AdvancedLoggerLib.inf | 1 - 2 files changed, 42 insertions(+), 89 deletions(-) diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c index 0cf4ad5217..8847119867 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c @@ -15,63 +15,13 @@ #include #include #include -#include #include "../AdvancedLoggerCommon.h" -#define ADV_LOGGER_MIN_SIZE (65536) - -STATIC ADVANCED_LOGGER_INFO *mLoggerInfo = NULL; -STATIC UINT32 mBufferSize = 0; -STATIC EFI_PHYSICAL_ADDRESS mMaxAddress = 0; -STATIC BOOLEAN mInitialized = FALSE; - -/** - Validate Info Blocks - - The address of the ADVANCE_LOGGER_INFO block pointer is captured during the first debug print. The - pointers LogBuffer and LogCurrent, and LogBufferSize, could be written to by untrusted code. Here, - we check that the pointers are within the allocated mLoggerInfo space, and that LogBufferSize, which - is used in multiple places to see if a new message will fit into the log buffer, is valid. - - @param NONE - - @return BOOLEAN TRUE = mLoggerInfo Block passes security checks - @return BOOLEAN FALSE= mLoggerInfo Block failed security checks - -**/ -STATIC -BOOLEAN -ValidateInfoBlock ( - VOID - ) -{ - if (mLoggerInfo == NULL) { - return FALSE; - } - - if (mLoggerInfo->Signature != ADVANCED_LOGGER_SIGNATURE) { - return FALSE; - } - - if (mLoggerInfo->LogBuffer != (PA_FROM_PTR (mLoggerInfo + 1))) { - return FALSE; - } - - if ((mLoggerInfo->LogCurrent > mMaxAddress) || - (mLoggerInfo->LogCurrent < mLoggerInfo->LogBuffer)) - { - return FALSE; - } - - if (mBufferSize == 0) { - mBufferSize = mLoggerInfo->LogBufferSize; - } else if (mLoggerInfo->LogBufferSize != mBufferSize) { - return FALSE; - } - - return TRUE; -} +// +// NO GLOBALS! This routine may run before data sections are writable, and so +// cannot presume globals will be available. +// /** The logger Information Block is carved from the Trust Zone at a specific fixed address. @@ -87,8 +37,8 @@ ValidateInfoBlock ( PcdAdvancedLoggerCarBase -- NOT USED, leave at default PcdAdvancedLoggerPreMemPages -- NOT USED, leave at default - NOTE: A debug statement here will cause recursion. Insure that the recursion will be - a straight path just to return the existing mLoggerInfo. + NOTE: A debug statement here will cause recursion. Avoid debug statements + or calls to external functions that may contain debug prints. @param - None @@ -102,48 +52,52 @@ AdvancedLoggerGetLoggerInfo ( VOID ) { - EFI_STATUS Status; - EFI_PHYSICAL_ADDRESS Address; + ADVANCED_LOGGER_INFO *LoggerInfo; + EFI_PHYSICAL_ADDRESS MaxAddress; ASSERT (FeaturePcdGet (PcdAdvancedLoggerFixedInRAM)); if (!FeaturePcdGet (PcdAdvancedLoggerFixedInRAM)) { return NULL; } - if (!mInitialized) { - // If this is the first time for MM core to get here, the memory attributes of this module - // may not be fully set yet. Thus set the memory for global variables attributes to RW first. - Address = ALIGN_VALUE ((EFI_PHYSICAL_ADDRESS)(UINTN)&mInitialized - EFI_PAGE_SIZE + 1, EFI_PAGE_SIZE); - Status = MmuSetAttributes (Address, EFI_PAGE_SIZE, EFI_MEMORY_XP); - ASSERT_EFI_ERROR (Status); - Status = MmuClearAttributes (Address, EFI_PAGE_SIZE, EFI_MEMORY_RO); - ASSERT_EFI_ERROR (Status); - - Address = ALIGN_VALUE ((EFI_PHYSICAL_ADDRESS)(UINTN)&mLoggerInfo - EFI_PAGE_SIZE + 1, EFI_PAGE_SIZE); - Status = MmuSetAttributes (Address, EFI_PAGE_SIZE, EFI_MEMORY_XP); - ASSERT_EFI_ERROR (Status); - Status = MmuClearAttributes (Address, EFI_PAGE_SIZE, EFI_MEMORY_RO); - ASSERT_EFI_ERROR (Status); - - mInitialized = TRUE; // Only allow initialization once - mLoggerInfo = (ADVANCED_LOGGER_INFO *)(VOID *)FixedPcdGet64 (PcdAdvancedLoggerBase); - ASSERT (mLoggerInfo != NULL); - if (mLoggerInfo == NULL) { - return NULL; - } - - mLoggerInfo->HwPrintLevel = FixedPcdGet32 (PcdAdvancedLoggerHdwPortDebugPrintErrorLevel); - - mMaxAddress = mLoggerInfo->LogBuffer + mLoggerInfo->LogBufferSize; - mBufferSize = mLoggerInfo->LogBufferSize; + LoggerInfo = (ADVANCED_LOGGER_INFO *)(VOID *)FixedPcdGet64 (PcdAdvancedLoggerBase); + if (LoggerInfo == NULL) { + return NULL; + } + + // + // The pointers LogBuffer and LogCurrent, and LogBufferSize, could be written + // to by untrusted code. Here, we check that the pointers are within the + // allocated LoggerInfo space, and that LogBufferSize, which is used in + // multiple places to see if a new message will fit into the log buffer, is + // valid. + // + + if (LoggerInfo->Signature != ADVANCED_LOGGER_SIGNATURE) { + return NULL; + } + + // Ensure the start of the log is in the correct location. + if (LoggerInfo->LogBuffer != (PA_FROM_PTR (LoggerInfo + 1))) { + return NULL; } - if (((mLoggerInfo) != NULL) && !ValidateInfoBlock ()) { - mLoggerInfo = NULL; - DEBUG ((DEBUG_ERROR, "%a: LoggerInfo marked invalid\n", __FUNCTION__)); + // Make sure the size of the buffer does not overrun it's fixed size. + MaxAddress = LoggerInfo->LogBuffer + LoggerInfo->LogBufferSize; + if ((MaxAddress - PA_FROM_PTR (LoggerInfo)) > + (FixedPcdGet32 (PcdAdvancedLoggerPages) * EFI_PAGE_SIZE)) + { + return NULL; + } + + // Ensure the current pointer does not overrun. + if ((LoggerInfo->LogCurrent > MaxAddress) || + (LoggerInfo->LogCurrent < LoggerInfo->LogBuffer)) + { + return NULL; } - return mLoggerInfo; + return LoggerInfo; } /** diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.inf index 7bf5cdb9dd..c35ef5274d 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.inf @@ -36,7 +36,6 @@ BaseMemoryLib DebugLib SynchronizationLib - MmuLib [Guids] gAdvancedLoggerHobGuid