From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=chao.b.zhang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1737A21F3C19C for ; Fri, 13 Oct 2017 08:51:02 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Oct 2017 08:54:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,371,1503385200"; d="scan'208";a="1024892539" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga003.jf.intel.com with ESMTP; 13 Oct 2017 08:54:33 -0700 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 13 Oct 2017 08:54:33 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 13 Oct 2017 08:53:22 -0700 Received: from shsmsx152.ccr.corp.intel.com ([169.254.6.93]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Fri, 13 Oct 2017 23:53:21 +0800 From: "Zhang, Chao B" To: "Chen, Chen A" , "edk2-devel@lists.01.org" CC: "Wu, Hao A" Thread-Topic: [PATCH] SecurityPkg/SecureBootConfigDxe: Change the declaring of buffer. Thread-Index: AQHTRBSiRcqEZND4Nki72Kr9drN7lKLh7k5w Date: Fri, 13 Oct 2017 15:53:20 +0000 Message-ID: References: <20171013111545.10776-1-chen.a.chen@intel.com> In-Reply-To: <20171013111545.10776-1-chen.a.chen@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] SecurityPkg/SecureBootConfigDxe: Change the declaring of buffer. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Oct 2017 15:51:03 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable Hi ChenChen=1B$B!'=1B(B It is good to replace magic code with Micro. Can you specify the reason= why use Stack instead of Heap? -----Original Message----- From: Chen, Chen A=20 Sent: Friday, October 13, 2017 7:16 PM To: edk2-devel@lists.01.org Cc: Chen, Chen A ; Zhang, Chao B ; Wu, Hao A Subject: [PATCH] SecurityPkg/SecureBootConfigDxe: Change the declaring of b= uffer. For known max length buffer, declaring there buffers as arrays instead of a= llocating from the heap. Cc: Zhang Chao B Cc: Wu Hao A Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: chenc2 --- .../SecureBootConfigDxe/SecureBootConfigImpl.c | 122 ++++++-----------= ---- .../SecureBootConfigDxe/SecureBootConfigImpl.h | 2 +- 2 files changed, 32 insertions(+), 92 deletions(-) diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBo= otConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/Secu= reBootConfigImpl.c index 3e03be9738..3ff90b5c8b 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi= gImpl.c +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo +++ nfigImpl.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; =20 Status =3D EFI_SUCCESS; - VariableName =3D NULL; VariableAttr =3D 0; VariableDataSize =3D 0; ListIndex =3D 0; @@ -3092,18 +3091,13 @@ DeleteSignatureEx ( VariableData =3D NULL; NewVariableData =3D NULL; =20 - VariableName =3D AllocateZeroPool (100); - if (VariableName =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - + ZeroMem (VariableName, sizeof (VariableName)); if (PrivateData->VariableName =3D=3D VARIABLE_DB) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE); } else if (PrivateData->VariableName =3D=3D VARIABLE_DBX) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE1); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE1); } else if (PrivateData->VariableName =3D=3D VARIABLE_DBT) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE2); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE2); } else { goto ON_EXIT; } @@ -3222,7 +3216,6 @@ DeleteSignatureEx ( } =20 ON_EXIT: - SECUREBOOT_FREE_NON_NULL (VariableName); SECUREBOOT_FREE_NON_NULL (VariableData); SECUREBOOT_FREE_NON_NULL (NewVariableData); =20 @@ -3594,9 +3587,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]; =20 Status =3D EFI_SUCCESS; StartOpCodeHandle =3D NULL; @@ -3605,9 +3598,6 @@ LoadSignatureList ( EndGotoHandle =3D NULL; Index =3D 0; VariableData =3D NULL; - VariableName =3D NULL; - NameBuffer =3D NULL; - HelpBuffer =3D NULL; =20 // // Initialize the container for dynamic opcodes. @@ -3675,20 +3665,15 @@ LoadSignatureList ( EndGoto->ExtendOpCode =3D EFI_IFR_EXTEND_OP_LABEL; EndGoto->Number =3D LABEL_END; =20 - VariableName =3D AllocateZeroPool (100); - if (VariableName =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - + ZeroMem (VariableName, sizeof (VariableName)); if (PrivateData->VariableName =3D=3D VARIABLE_DB) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE); DstFormId =3D FORMID_SECURE_BOOT_DB_OPTION_FORM; } else if (PrivateData->VariableName =3D=3D VARIABLE_DBX) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE1); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE1); DstFormId =3D FORMID_SECURE_BOOT_DBX_OPTION_FORM; } else if (PrivateData->VariableName =3D=3D VARIABLE_DBT) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE2); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE2); DstFormId =3D FORMID_SECURE_BOOT_DBT_OPTION_FORM; } else { goto ON_EXIT; @@ -3722,18 +3707,6 @@ LoadSignatureList ( goto ON_EXIT; } =20 - NameBuffer =3D AllocateZeroPool (100); - if (NameBuffer =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - - HelpBuffer =3D AllocateZeroPool (100); - if (HelpBuffer =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - RemainingSize =3D DataSize; ListWalker =3D (EFI_SIGNATURE_LIST *)VariableData; while ((RemainingSize > 0) && (RemainingSize >=3D ListWalker->SignatureL= istSize)) { @@ -3755,13 +3728,16 @@ LoadSignatureList ( ListType =3D STRING_TOKEN (STR_LIST_TYPE_UNKNOWN); } =20 + ZeroMem (NameBuffer, sizeof (NameBuffer)); UnicodeSPrint (NameBuffer, - 100, + sizeof (NameBuffer), HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LI= ST_NAME_FORMAT), NULL), Index + 1 ); + + ZeroMem (HelpBuffer, sizeof (HelpBuffer)); UnicodeSPrint (HelpBuffer, - 100, + sizeof (HelpBuffer), HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LI= ST_HELP_FORMAT), NULL), HiiGetString (PrivateData->HiiHandle, ListType, NULL), SIGNATURE_DATA_COUNTS (ListWalker) @@ -3776,9 +3752,6 @@ LoadSignatu= reList ( QuestionIdBase + Index++ ); =20 - ZeroMem (NameBuffer, 100); - ZeroMem (HelpBuffer, 100); - RemainingSize -=3D ListWalker->SignatureListSize; ListWalker =3D (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + ListWalker= ->SignatureListSize); } @@ -3805,10 +3778,7 @@ ON_EXIT: SECUREBOOT_FREE_NON_OPCODE (StartGotoHandle); SECUREBOOT_FREE_NON_OPCODE (EndGotoHandle); =20 - SECUREBOOT_FREE_NON_NULL (VariableName); SECUREBOOT_FREE_NON_NULL (VariableData); - SECUREBOOT_FREE_NON_NULL (NameBuffer); - SECUREBOOT_FREE_NON_NULL (HelpBuffer); =20 PrivateData->ListCount =3D Index; =20 @@ -3957,18 +3927,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; =20 Status =3D EFI_SUCCESS; Time =3D NULL; HelpInfoIndex =3D 0; - GuidString =3D NULL; DataString =3D NULL; - TimeString =3D NULL; HelpInfoString =3D NULL; IsCert =3D FALSE; =20 @@ -4003,12 +3971,6 @@ FormatHelpInfo ( goto ON_EXIT; } =20 - GuidString =3D AllocateZeroPool (100); - if (GuidString =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - TotalSize =3D 1024; HelpInfoString =3D AllocateZeroPool (TotalSize); if (HelpInfoString =3D=3D NULL) { @@ -4019,7 +3981,8 @@ FormatHelpInfo ( // // Format GUID part. // - GuidToString(&DataEntry->SignatureOwner, GuidString, 100); + ZeroMem (GuidString, sizeof (GuidString)); =20 + GuidToString(&DataEntry->SignatureOwner, GuidString, BUFFER_MAX_SIZE); HelpInfoIndex +=3D UnicodeSPrint ( &HelpInfoString[HelpInfoIndex], TotalSize - sizeof(CHAR16) * HelpInfoIndex, @@ -4059,= 15 +4022,10 @@ FormatHelpInfo ( // Format revocation time part. // if (Time !=3D NULL) { - TimeString =3D AllocateZeroPool(100); - if (TimeString =3D=3D NULL) { - Status =3D 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 +4044,8 @@ FormatHelpInfo ( } =20 *StringId =3D HiiSetString (PrivateData->HiiHandle, 0, HelpInfoString, N= ULL); - ON_EXIT: - SECUREBOOT_FREE_NON_NULL (GuidString); SECUREBOOT_FREE_NON_NULL (DataString); - SECUREBOOT_FREE_NON_NULL (TimeString); SECUREBOOT_FREE_NON_NULL (HelpInfoString); =20 return Status; @@ -4129,16 +4084,14 @@ LoadSignatureData ( UINTN RemainingSize; UINT16 Index; UINT8 *VariableData; - CHAR16 *VariableName; - CHAR16 *NameBuffer; + CHAR16 VariableName[BUFFER_MAX_SIZE]; + CHAR16 NameBuffer[BUFFER_MAX_SIZE]; =20 Status =3D EFI_SUCCESS; StartOpCodeHandle =3D NULL; EndOpCodeHandle =3D NULL; Index =3D 0; VariableData =3D NULL; - VariableName =3D NULL; - NameBuffer =3D NULL; =20 // // Initialize the container for dynamic opcodes. @@ -4176,18 +4129,13 @@ LoadSignatureData ( EndLabel->ExtendOpCode =3D EFI_IFR_EXTEND_OP_LABEL; EndLabel->Number =3D LABEL_END; =20 - VariableName =3D AllocateZeroPool (100); - if (VariableName =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - + ZeroMem (VariableName, sizeof (VariableName)); if (PrivateData->VariableName =3D=3D VARIABLE_DB) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE); } else if (PrivateData->VariableName =3D=3D VARIABLE_DBX) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE1); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE1); } else if (PrivateData->VariableName =3D=3D VARIABLE_DBT) { - UnicodeSPrint (VariableName, 100, EFI_IMAGE_SECURITY_DATABASE2); + UnicodeSPrint (VariableName, sizeof (VariableName),=20 + EFI_IMAGE_SECURITY_DATABASE2); } else { goto ON_EXIT; } @@ -4211,12 +4159,6 @@ LoadSignatureData ( goto ON_EXIT; } =20 - NameBuffer =3D AllocateZeroPool (100); - if (NameBuffer =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto ON_EXIT; - } - RemainingSize =3D DataSize; ListWalker =3D (EFI_SIGNATURE_LIST *)VariableData; =20 @@ -4233,8 +4175,9 @@ LoadSignatureData ( // // Format name buffer. // + ZeroMem (NameBuffer, sizeof (NameBuffer)); UnicodeSPrint (NameBuffer, - 100, + sizeof (NameBuffer), HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DA= TA_NAME_FORMAT), NULL), Index + 1 ); @@ -4268,7 +4211,6 @@ LoadSignatureData ( // This memory buffer will be freed when exit from the SECUREBOOT_DELETE= _SIGNATURE_DATA_FORM form. // PrivateData->CheckArray =3D AllocateZeroPool (SIGNATURE_DATA_COUNTS (Lis= tWalker) * sizeof (BOOLEAN)); - ON_EXIT: HiiUpdateForm ( PrivateData->HiiHandle, @@ -4281,9 +4223,7 @@ ON_EXIT: SECUREBOOT_FREE_NON_OPCODE (StartOpCodeHandle); SECUREBOOT_FREE_NON_OPCODE (EndOpCodeHandle); =20 - SECUREBOOT_FREE_NON_NULL (VariableName); SECUREBOOT_FREE_NON_NULL (VariableData); - SECUREBOOT_FREE_NON_NULL (NameBuffer); =20 return Status; } diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBo= otConfigImpl.h b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/Secu= reBootConfigImpl.h index 52ad91b002..6082fb7df2 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi= gImpl.h +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo +++ nfigImpl.h @@ -67,7 +67,7 @@ extern EFI_IFR_GUID_LABEL *mEndLabel; =20 #define MAX_CHAR 480 #define TWO_BYTE_ENCODE 0x82 - +#define BUFFER_MAX_SIZE 100 =20 // // SHA-256 digest size in bytes -- 2.13.2.windows.1