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.43; helo=mga05.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 9971820945C00 for ; Mon, 9 Oct 2017 19:43:18 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 09 Oct 2017 19:46:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,502,1500966000"; d="scan'208";a="321400483" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 09 Oct 2017 19:46:45 -0700 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 9 Oct 2017 19:46:45 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 9 Oct 2017 19:46:45 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Tue, 10 Oct 2017 10:46:43 +0800 From: "Wu, Hao A" To: "Chen, Chen A" , "edk2-devel@lists.01.org" CC: "Zhang, Chao B" Thread-Topic: [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference. Thread-Index: AQHTQWypDnvgwN75L0GpphAp7ocgFKLcWfYA Date: Tue, 10 Oct 2017 02:46:42 +0000 Message-ID: References: <20171010020807.16452-1-chen.a.chen@intel.com> In-Reply-To: <20171010020807.16452-1-chen.a.chen@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference. 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: Tue, 10 Oct 2017 02:43:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Some comments below. Best Regards, Hao Wu > -----Original Message----- > From: Chen, Chen A > Sent: Tuesday, October 10, 2017 10:08 AM > To: edk2-devel@lists.01.org > Cc: Chen, Chen A; Zhang, Chao B; Wu, Hao A > Subject: [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NU= LL > dereference. >=20 > Cc: Zhang Chao > Cc: Wu Hao > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Chen A Chen > --- > .../SecureBootConfigDxe/SecureBootConfigImpl.c | 80 +++++++++++++++-= --- > --- > 1 file changed, 57 insertions(+), 23 deletions(-) >=20 > diff --git > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > index 3e03be9738..457e020ece 100644 > --- > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > +++ > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > @@ -3579,7 +3579,9 @@ LoadSignatureList ( > ) > { > EFI_STATUS Status; > - EFI_STRING_ID ListType; > + EFI_STRING EfiStringTemp1; > + EFI_STRING EfiStringTemp2; > + EFI_STRING_ID ListType;; An extra ';' is used here. > EFI_SIGNATURE_LIST *ListWalker; > EFI_IFR_GUID_LABEL *StartLabel; > EFI_IFR_GUID_LABEL *EndLabel; > @@ -3599,6 +3601,8 @@ LoadSignatureList ( > CHAR16 *HelpBuffer; >=20 > Status =3D EFI_SUCCESS; > + EfiStringTemp1 =3D NULL; > + EfiStringTemp2 =3D NULL; > StartOpCodeHandle =3D NULL; > EndOpCodeHandle =3D NULL; > StartGotoHandle =3D NULL; > @@ -3755,17 +3759,19 @@ LoadSignatureList ( > ListType =3D STRING_TOKEN (STR_LIST_TYPE_UNKNOWN); > } >=20 > - UnicodeSPrint (NameBuffer, > - 100, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_LIST_NAME_FORMAT), NULL), > - Index + 1 > - ); > - UnicodeSPrint (HelpBuffer, > - 100, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_LIST_HELP_FORMAT), NULL), > - HiiGetString (PrivateData->HiiHandle, ListType, NULL), > - SIGNATURE_DATA_COUNTS (ListWalker) > - ); > + EfiStringTemp1 =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_LIST_NAME_FORMAT), NULL); > + if (EfiStringTemp1 =3D=3D NULL) { > + goto ON_EXIT; > + } > + UnicodeSPrint (NameBuffer, 100, EfiStringTemp1, Index + 1); > + EfiStringTemp1 =3D NULL; Before setting 'EfiStringTemp1' to NULL, I think the returned string from HiiGetString() should be freed first. There are many similar cases in the codes. Also, could you help to replace the usages of '100' like: * VariableName =3D AllocateZeroPool (100); * NameBuffer =3D AllocateZeroPool (100); * UnicodeSPrint (NameBuffer, 100, EfiStringTemp1, Index + 1); ... with a macro? It will be helpful for maintaining the codes. And how about declaring string buffers like 'VariableName', 'NameBuffer' as arrays instead of allocating them on the heap? > + > + EfiStringTemp1 =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_LIST_HELP_FORMAT), NULL); > + EfiStringTemp2 =3D HiiGetString (PrivateData->HiiHandle, ListType, N= ULL); > + if (EfiStringTemp1 =3D=3D NULL || EfiStringTemp2 =3D=3D NULL) { > + goto ON_EXIT; > + } > + UnicodeSPrint (HelpBuffer, 100, EfiStringTemp1, EfiStringTemp2, > SIGNATURE_DATA_COUNTS (ListWalker)); >=20 > HiiCreateGotoOpCode ( > StartOpCodeHandle, > @@ -3953,6 +3959,8 @@ FormatHelpInfo ( > { > EFI_STATUS Status; > EFI_TIME *Time; > + EFI_STRING EfiStringTemp1; > + EFI_STRING EfiStringTemp2; > EFI_STRING_ID ListTypeId; > UINTN DataSize; > UINTN HelpInfoIndex; > @@ -3964,6 +3972,8 @@ FormatHelpInfo ( > BOOLEAN IsCert; >=20 > Status =3D EFI_SUCCESS; > + EfiStringTemp1 =3D NULL; > + EfiStringTemp2 =3D NULL; > Time =3D NULL; > HelpInfoIndex =3D 0; > GuidString =3D NULL; > @@ -4016,6 +4026,10 @@ FormatHelpInfo ( > goto ON_EXIT; > } >=20 > + EfiStringTemp1 =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL); > + if (EfiStringTemp1 =3D=3D NULL) { > + goto ON_EXIT; > + } > // > // Format GUID part. > // > @@ -4023,20 +4037,29 @@ FormatHelpInfo ( > HelpInfoIndex +=3D UnicodeSPrint ( > &HelpInfoString[HelpInfoIndex], > TotalSize - sizeof(CHAR16) * HelpInfoIndex, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL), > + EfiStringTemp1, > GuidString > ); > + EfiStringTemp1 =3D NULL; >=20 > + EfiStringTemp2 =3D HiiGetString (PrivateData->HiiHandle, ListTypeId, N= ULL); > + if (EfiStringTemp2 =3D=3D NULL) { > + goto ON_EXIT; > + } > // > // Format content part, it depends on the type of signature list, hash= value or > CN. > // > if (IsCert) { > GetCommonNameFromX509 (ListEntry, DataEntry, &DataString); > + EfiStringTemp1 =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_HELP_FORMAT_CN), NULL); > + if (EfiStringTemp1 =3D=3D NULL) { > + goto ON_EXIT; > + } > HelpInfoIndex +=3D UnicodeSPrint( > &HelpInfoString[HelpInfoIndex], > TotalSize - sizeof(CHAR16) * HelpInfoIndex, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_HELP_FORMAT_CN), NULL), > - HiiGetString (PrivateData->HiiHandle, ListTypeId,= NULL), > + EfiStringTemp1, > + EfiStringTemp2, > DataSize, > DataString > ); > @@ -4045,15 +4068,20 @@ FormatHelpInfo ( > // Format hash value for each signature data entry. > // > ParseHashValue (ListEntry, DataEntry, &DataString); > + EfiStringTemp1 =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL); > + if (EfiStringTemp1 =3D=3D NULL) { > + goto ON_EXIT; > + } > HelpInfoIndex +=3D UnicodeSPrint ( > &HelpInfoString[HelpInfoIndex], > TotalSize - sizeof(CHAR16) * HelpInfoIndex, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL), > - HiiGetString (PrivateData->HiiHandle, ListTypeId,= NULL), > + EfiStringTemp1, > + EfiStringTemp2, > DataSize, > DataString > ); > } > + EfiStringTemp1 =3D NULL; >=20 > // > // Format revocation time part. > @@ -4077,10 +4105,14 @@ FormatHelpInfo ( > Time->Second > ); >=20 > + EfiStringTemp1 =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_HELP_FORMAT_TIME), NULL); > + if (EfiStringTemp1 =3D=3D NULL) { > + goto ON_EXIT; > + } > UnicodeSPrint ( > &HelpInfoString[HelpInfoIndex], > TotalSize - sizeof (CHAR16) * HelpInfoIndex, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_HELP_FORMAT_TIME), NULL), > + EfiStringTemp1, > TimeString > ); > } > @@ -4122,6 +4154,7 @@ LoadSignatureData ( > EFI_SIGNATURE_DATA *DataWalker; > EFI_IFR_GUID_LABEL *StartLabel; > EFI_IFR_GUID_LABEL *EndLabel; > + EFI_STRING EfiString; > EFI_STRING_ID HelpStringId; > VOID *StartOpCodeHandle; > VOID *EndOpCodeHandle; > @@ -4133,6 +4166,7 @@ LoadSignatureData ( > CHAR16 *NameBuffer; >=20 > Status =3D EFI_SUCCESS; > + EfiString =3D NULL; > StartOpCodeHandle =3D NULL; > EndOpCodeHandle =3D NULL; > Index =3D 0; > @@ -4229,15 +4263,15 @@ LoadSignatureData ( > } >=20 > DataWalker =3D (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker + > sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize); > + EfiString =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_NAME_FORMAT), NULL); > + if (EfiString =3D=3D NULL) { > + goto ON_EXIT; > + } > for (Index =3D 0; Index < SIGNATURE_DATA_COUNTS(ListWalker); Index =3D= Index > + 1) { > // > // Format name buffer. > // > - UnicodeSPrint (NameBuffer, > - 100, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_NAME_FORMAT), NULL), > - Index + 1 > - ); > + UnicodeSPrint (NameBuffer, 100, EfiString, Index + 1); >=20 > // > // Format help info buffer. > -- > 2.13.2.windows.1