From 65c77f02104cf0cf7bd224df3a5fc08795df9aad Mon Sep 17 00:00:00 2001 From: chenc2 Date: Tue, 17 Oct 2017 16:45:06 +0800 Subject: [PATCH 1/1] SecurityPkg/SecureBootConfigDxe: Change the declaring of buffer. The change doesn't impact the functionality. To avoid magic code is helpful for maintaining the codes. Use stack variable for known max length variable is more clear and safe than heap buffer. Cc: Zhang Chao B Cc: Wu Hao A Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: chenc2 Reviewed-by: Chao Zhang --- .../SecureBootConfigImpl.c | 119 +++++------------- .../SecureBootConfigImpl.h | 2 +- 2 files changed, 29 insertions(+), 92 deletions(-) diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c index 161512a1f4..acb0dc0558 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c @@ -3073,7 +3073,7 @@ DeleteSignatureEx ( EFI_SIGNATURE_LIST *ListWalker; EFI_SIGNATURE_LIST *NewCertList; EFI_SIGNATURE_DATA *DataWalker; - CHAR16 *VariableName; + CHAR16 VariableName[BUFFER_MAX_SIZE]; UINT32 VariableAttr; UINTN VariableDataSize; UINTN RemainingSize; @@ -3084,7 +3084,6 @@ DeleteSignatureEx ( UINT8 *NewVariableData; Status = EFI_SUCCESS; - VariableName = NULL; VariableAttr = 0; VariableDataSize = 0; ListIndex = 0; @@ -3092,18 +3091,12 @@ DeleteSignatureEx ( VariableData = NULL; NewVariableData = NULL; - VariableName = AllocateZeroPool (100); - if (VariableName == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - if (PrivateData->VariableName == Variable_DB) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE); } else if (PrivateData->VariableName == Variable_DBX) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE1); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE1); } else if (PrivateData->VariableName == Variable_DBT) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE2); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE2); } else { goto ON_EXIT; } @@ -3222,7 +3215,6 @@ DeleteSignatureEx ( } ON_EXIT: - SECUREBOOT_FREE_NON_NULL (VariableName); SECUREBOOT_FREE_NON_NULL (VariableData); SECUREBOOT_FREE_NON_NULL (NewVariableData); @@ -3594,9 +3586,9 @@ LoadSignatureList ( UINTN RemainingSize; UINT16 Index; UINT8 *VariableData; - CHAR16 *VariableName; - CHAR16 *NameBuffer; - CHAR16 *HelpBuffer; + CHAR16 VariableName[BUFFER_MAX_SIZE]; + CHAR16 NameBuffer[BUFFER_MAX_SIZE]; + CHAR16 HelpBuffer[BUFFER_MAX_SIZE]; Status = EFI_SUCCESS; StartOpCodeHandle = NULL; @@ -3605,9 +3597,6 @@ LoadSignatureList ( EndGotoHandle = NULL; Index = 0; VariableData = NULL; - VariableName = NULL; - NameBuffer = NULL; - HelpBuffer = NULL; // // Initialize the container for dynamic opcodes. @@ -3675,20 +3664,14 @@ LoadSignatureList ( EndGoto->ExtendOpCode = EFI_IFR_EXTEND_OP_LABEL; EndGoto->Number = LABEL_END; - VariableName = AllocateZeroPool (100); - if (VariableName == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - if (PrivateData->VariableName == Variable_DB) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE); DstFormId = FORMID_SECURE_BOOT_DB_OPTION_FORM; } else if (PrivateData->VariableName == Variable_DBX) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE1); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE1); DstFormId = FORMID_SECURE_BOOT_DBX_OPTION_FORM; } else if (PrivateData->VariableName == Variable_DBT) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE2); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE2); DstFormId = FORMID_SECURE_BOOT_DBT_OPTION_FORM; } else { goto ON_EXIT; @@ -3722,18 +3705,6 @@ LoadSignatureList ( goto ON_EXIT; } - NameBuffer = AllocateZeroPool (100); - if (NameBuffer == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - - HelpBuffer = AllocateZeroPool (100); - if (HelpBuffer == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - RemainingSize = DataSize; ListWalker = (EFI_SIGNATURE_LIST *)VariableData; while ((RemainingSize > 0) && (RemainingSize >= ListWalker->SignatureListSize)) { @@ -3755,13 +3726,16 @@ LoadSignatureList ( ListType = STRING_TOKEN (STR_LIST_TYPE_UNKNOWN); } + ZeroMem (NameBuffer, sizeof (NameBuffer)); UnicodeSPrint (NameBuffer, - 100, + sizeof (NameBuffer), HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LIST_NAME_FORMAT), NULL), Index + 1 ); + + ZeroMem (HelpBuffer, sizeof (HelpBuffer)); UnicodeSPrint (HelpBuffer, - 100, + sizeof (HelpBuffer), HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LIST_HELP_FORMAT), NULL), HiiGetString (PrivateData->HiiHandle, ListType, NULL), SIGNATURE_DATA_COUNTS (ListWalker) @@ -3776,9 +3750,6 @@ LoadSignatureList ( QuestionIdBase + Index++ ); - ZeroMem (NameBuffer, 100); - ZeroMem (HelpBuffer, 100); - RemainingSize -= ListWalker->SignatureListSize; ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + ListWalker->SignatureListSize); } @@ -3805,10 +3776,7 @@ ON_EXIT: SECUREBOOT_FREE_NON_OPCODE (StartGotoHandle); SECUREBOOT_FREE_NON_OPCODE (EndGotoHandle); - SECUREBOOT_FREE_NON_NULL (VariableName); SECUREBOOT_FREE_NON_NULL (VariableData); - SECUREBOOT_FREE_NON_NULL (NameBuffer); - SECUREBOOT_FREE_NON_NULL (HelpBuffer); PrivateData->ListCount = Index; @@ -3957,18 +3925,16 @@ FormatHelpInfo ( UINTN DataSize; UINTN HelpInfoIndex; UINTN TotalSize; - CHAR16 *GuidString; + CHAR16 GuidString[BUFFER_MAX_SIZE]; + CHAR16 TimeString[BUFFER_MAX_SIZE]; CHAR16 *DataString; - CHAR16 *TimeString; CHAR16 *HelpInfoString; BOOLEAN IsCert; Status = EFI_SUCCESS; Time = NULL; HelpInfoIndex = 0; - GuidString = NULL; DataString = NULL; - TimeString = NULL; HelpInfoString = NULL; IsCert = FALSE; @@ -4003,12 +3969,6 @@ FormatHelpInfo ( goto ON_EXIT; } - GuidString = AllocateZeroPool (100); - if (GuidString == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - TotalSize = 1024; HelpInfoString = AllocateZeroPool (TotalSize); if (HelpInfoString == NULL) { @@ -4019,7 +3979,8 @@ FormatHelpInfo ( // // Format GUID part. // - GuidToString(&DataEntry->SignatureOwner, GuidString, 100); + ZeroMem (GuidString, sizeof (GuidString)); + GuidToString(&DataEntry->SignatureOwner, GuidString, BUFFER_MAX_SIZE); HelpInfoIndex += UnicodeSPrint ( &HelpInfoString[HelpInfoIndex], TotalSize - sizeof(CHAR16) * HelpInfoIndex, @@ -4059,15 +4020,10 @@ FormatHelpInfo ( // Format revocation time part. // if (Time != NULL) { - TimeString = AllocateZeroPool(100); - if (TimeString == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - + ZeroMem (TimeString, sizeof (TimeString)); UnicodeSPrint ( TimeString, - 100, + sizeof (TimeString), L"%d-%d-%d %d:%d:%d", Time->Year, Time->Month, @@ -4086,11 +4042,8 @@ FormatHelpInfo ( } *StringId = HiiSetString (PrivateData->HiiHandle, 0, HelpInfoString, NULL); - ON_EXIT: - SECUREBOOT_FREE_NON_NULL (GuidString); SECUREBOOT_FREE_NON_NULL (DataString); - SECUREBOOT_FREE_NON_NULL (TimeString); SECUREBOOT_FREE_NON_NULL (HelpInfoString); return Status; @@ -4129,16 +4082,14 @@ LoadSignatureData ( UINTN RemainingSize; UINT16 Index; UINT8 *VariableData; - CHAR16 *VariableName; - CHAR16 *NameBuffer; + CHAR16 VariableName[BUFFER_MAX_SIZE]; + CHAR16 NameBuffer[BUFFER_MAX_SIZE]; Status = EFI_SUCCESS; StartOpCodeHandle = NULL; EndOpCodeHandle = NULL; Index = 0; VariableData = NULL; - VariableName = NULL; - NameBuffer = NULL; // // Initialize the container for dynamic opcodes. @@ -4176,18 +4127,12 @@ LoadSignatureData ( EndLabel->ExtendOpCode = EFI_IFR_EXTEND_OP_LABEL; EndLabel->Number = LABEL_END; - VariableName = AllocateZeroPool (100); - if (VariableName == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - if (PrivateData->VariableName == Variable_DB) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE); } else if (PrivateData->VariableName == Variable_DBX) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE1); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE1); } else if (PrivateData->VariableName == Variable_DBT) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE2); + UnicodeSPrint (VariableName, sizeof (VariableName), EFI_IMAGE_SECURITY_DATABASE2); } else { goto ON_EXIT; } @@ -4211,12 +4156,6 @@ LoadSignatureData ( goto ON_EXIT; } - NameBuffer = AllocateZeroPool (100); - if (NameBuffer == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - RemainingSize = DataSize; ListWalker = (EFI_SIGNATURE_LIST *)VariableData; @@ -4233,8 +4172,9 @@ LoadSignatureData ( // // Format name buffer. // + ZeroMem (NameBuffer, sizeof (NameBuffer)); UnicodeSPrint (NameBuffer, - 100, + sizeof (NameBuffer), HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_NAME_FORMAT), NULL), Index + 1 ); @@ -4268,7 +4208,6 @@ LoadSignatureData ( // This memory buffer will be freed when exit from the SECUREBOOT_DELETE_SIGNATURE_DATA_FORM form. // PrivateData->CheckArray = AllocateZeroPool (SIGNATURE_DATA_COUNTS (ListWalker) * sizeof (BOOLEAN)); - ON_EXIT: HiiUpdateForm ( PrivateData->HiiHandle, @@ -4281,9 +4220,7 @@ ON_EXIT: SECUREBOOT_FREE_NON_OPCODE (StartOpCodeHandle); SECUREBOOT_FREE_NON_OPCODE (EndOpCodeHandle); - SECUREBOOT_FREE_NON_NULL (VariableName); SECUREBOOT_FREE_NON_NULL (VariableData); - SECUREBOOT_FREE_NON_NULL (NameBuffer); return Status; } diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h index e8ae992dd2..f4935965a9 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h @@ -67,7 +67,7 @@ extern EFI_IFR_GUID_LABEL *mEndLabel; #define MAX_CHAR 480 #define TWO_BYTE_ENCODE 0x82 - +#define BUFFER_MAX_SIZE 100 // // SHA-256 digest size in bytes -- 2.39.2