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.88; helo=mga01.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 38A12202E5CFC for ; Wed, 18 Oct 2017 01:03:22 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Oct 2017 01:06:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,395,1503385200"; d="scan'208";a="164417788" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga006.fm.intel.com with ESMTP; 18 Oct 2017 01:06:58 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 18 Oct 2017 01:06:59 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 18 Oct 2017 01:06:58 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by shsmsx102.ccr.corp.intel.com ([169.254.2.175]) with mapi id 14.03.0319.002; Wed, 18 Oct 2017 16:06:56 +0800 From: "Wu, Hao A" To: "Chen, Chen A" , "edk2-devel@lists.01.org" CC: "Zhang, Chao B" Thread-Topic: [PATCH v2] SecurityPkg/SecureBootConfigDxe: Handle lack of STR_SIGNATURE_* tokens Thread-Index: AQHTR+fys7ClBStVaUq5qJauMwibGaLpQDvQ Date: Wed, 18 Oct 2017 08:06:56 +0000 Message-ID: References: <20171018080548.5608-1-chen.a.chen@intel.com> In-Reply-To: <20171018080548.5608-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 v2] SecurityPkg/SecureBootConfigDxe: Handle lack of STR_SIGNATURE_* tokens 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: Wed, 18 Oct 2017 08:03:22 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Hao Wu Best Regards, Hao Wu > -----Original Message----- > From: Chen, Chen A > Sent: Wednesday, October 18, 2017 4:06 PM > To: edk2-devel@lists.01.org > Cc: Chen, Chen A; Zhang, Chao B; Wu, Hao A > Subject: [PATCH v2] SecurityPkg/SecureBootConfigDxe: Handle lack of > STR_SIGNATURE_* tokens >=20 > Add check to avoid NULL ptr dereference. The function HiiGetString > will return NULL pointer when the platform does not install the > appropriate string or call HiiGetString fail.(For example, HII not > support specified language.) >=20 > Cc: Zhang Chao > Cc: Wu Hao > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: chenc2 > --- > .../SecureBootConfigDxe/SecureBootConfigImpl.c | 113 ++++++++++++++-= --- > --- > 1 file changed, 76 insertions(+), 37 deletions(-) >=20 > diff --git > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > index acb0dc0558..d035763106 100644 > --- > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > +++ > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI > mpl.c > @@ -3572,6 +3572,9 @@ LoadSignatureList ( > { > EFI_STATUS Status; > EFI_STRING_ID ListType; > + EFI_STRING FormatNameString; > + EFI_STRING FormatHelpString; > + EFI_STRING FormatTypeString; > EFI_SIGNATURE_LIST *ListWalker; > EFI_IFR_GUID_LABEL *StartLabel; > EFI_IFR_GUID_LABEL *EndLabel; > @@ -3591,6 +3594,8 @@ LoadSignatureList ( > CHAR16 HelpBuffer[BUFFER_MAX_SIZE]; >=20 > Status =3D EFI_SUCCESS; > + FormatNameString =3D NULL; > + FormatHelpString =3D NULL; > StartOpCodeHandle =3D NULL; > EndOpCodeHandle =3D NULL; > StartGotoHandle =3D NULL; > @@ -3705,6 +3710,12 @@ LoadSignatureList ( > goto ON_EXIT; > } >=20 > + FormatNameString =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_LIST_NAME_FORMAT), NULL); > + FormatHelpString =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_LIST_HELP_FORMAT), NULL); > + if (FormatNameString =3D=3D NULL || FormatHelpString =3D=3D NULL) { > + goto ON_EXIT; > + } > + > RemainingSize =3D DataSize; > ListWalker =3D (EFI_SIGNATURE_LIST *)VariableData; > while ((RemainingSize > 0) && (RemainingSize >=3D ListWalker- > >SignatureListSize)) { > @@ -3725,21 +3736,23 @@ LoadSignatureList ( > } else { > ListType =3D STRING_TOKEN (STR_LIST_TYPE_UNKNOWN); > } > + FormatTypeString =3D HiiGetString (PrivateData->HiiHandle, ListType,= NULL); > + if (FormatTypeString =3D=3D NULL) { > + goto ON_EXIT; > + } >=20 > ZeroMem (NameBuffer, sizeof (NameBuffer)); > - UnicodeSPrint (NameBuffer, > - sizeof (NameBuffer), > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_LIST_NAME_FORMAT), NULL), > - Index + 1 > - ); > + UnicodeSPrint (NameBuffer, sizeof (NameBuffer), FormatNameString, In= dex > + 1); >=20 > ZeroMem (HelpBuffer, sizeof (HelpBuffer)); > UnicodeSPrint (HelpBuffer, > sizeof (HelpBuffer), > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_LIST_HELP_FORMAT), NULL), > - HiiGetString (PrivateData->HiiHandle, ListType, NULL), > + FormatHelpString, > + FormatTypeString, > SIGNATURE_DATA_COUNTS (ListWalker) > ); > + SECUREBOOT_FREE_NON_NULL (FormatTypeString); > + FormatTypeString =3D NULL; >=20 > HiiCreateGotoOpCode ( > StartOpCodeHandle, > @@ -3777,6 +3790,8 @@ ON_EXIT: > SECUREBOOT_FREE_NON_OPCODE (EndGotoHandle); >=20 > SECUREBOOT_FREE_NON_NULL (VariableData); > + SECUREBOOT_FREE_NON_NULL (FormatNameString); > + SECUREBOOT_FREE_NON_NULL (FormatHelpString); >=20 > PrivateData->ListCount =3D Index; >=20 > @@ -3922,6 +3937,8 @@ FormatHelpInfo ( > EFI_STATUS Status; > EFI_TIME *Time; > EFI_STRING_ID ListTypeId; > + EFI_STRING FormatHelpString; > + EFI_STRING FormatTypeString; > UINTN DataSize; > UINTN HelpInfoIndex; > UINTN TotalSize; > @@ -3931,12 +3948,13 @@ FormatHelpInfo ( > CHAR16 *HelpInfoString; > BOOLEAN IsCert; >=20 > - Status =3D EFI_SUCCESS; > - Time =3D NULL; > - HelpInfoIndex =3D 0; > - DataString =3D NULL; > - HelpInfoString =3D NULL; > - IsCert =3D FALSE; > + Status =3D EFI_SUCCESS; > + Time =3D NULL; > + FormatTypeString =3D NULL; > + HelpInfoIndex =3D 0; > + DataString =3D NULL; > + HelpInfoString =3D NULL; > + IsCert =3D FALSE; >=20 > if (CompareGuid(&ListEntry->SignatureType, &gEfiCertRsa2048Guid)) { > ListTypeId =3D STRING_TOKEN(STR_LIST_TYPE_RSA2048_SHA256); > @@ -3969,6 +3987,11 @@ FormatHelpInfo ( > goto ON_EXIT; > } >=20 > + FormatTypeString =3D HiiGetString (PrivateData->HiiHandle, ListTypeId,= NULL); > + if (FormatTypeString =3D=3D NULL) { > + goto ON_EXIT; > + } > + > TotalSize =3D 1024; > HelpInfoString =3D AllocateZeroPool (TotalSize); > if (HelpInfoString =3D=3D NULL) { > @@ -3981,40 +4004,45 @@ FormatHelpInfo ( > // > ZeroMem (GuidString, sizeof (GuidString)); > GuidToString(&DataEntry->SignatureOwner, GuidString, BUFFER_MAX_SIZE); > + FormatHelpString =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL); > + if (FormatHelpString =3D=3D NULL) { > + goto ON_EXIT; > + } > HelpInfoIndex +=3D UnicodeSPrint ( > &HelpInfoString[HelpInfoIndex], > TotalSize - sizeof(CHAR16) * HelpInfoIndex, > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL), > + FormatHelpString, > GuidString > ); > + SECUREBOOT_FREE_NON_NULL (FormatHelpString); > + FormatHelpString =3D NULL; >=20 > // > // Format content part, it depends on the type of signature list, hash= value or > CN. > // > if (IsCert) { > GetCommonNameFromX509 (ListEntry, DataEntry, &DataString); > - 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), > - DataSize, > - DataString > - ); > + FormatHelpString =3D HiiGetString (PrivateData->HiiHandle, STRING_TO= KEN > (STR_SIGNATURE_DATA_HELP_FORMAT_CN), NULL); > } else { > // > // Format hash value for each signature data entry. > // > ParseHashValue (ListEntry, DataEntry, &DataString); > - 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), > - DataSize, > - DataString > - ); > + FormatHelpString =3D HiiGetString (PrivateData->HiiHandle, STRING_TO= KEN > (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL); > + } > + if (FormatHelpString =3D=3D NULL) { > + goto ON_EXIT; > } > + HelpInfoIndex +=3D UnicodeSPrint ( > + &HelpInfoString[HelpInfoIndex], > + TotalSize - sizeof (CHAR16) * HelpInfoIndex, > + FormatHelpString, > + FormatTypeString, > + DataSize, > + DataString > + ); > + SECUREBOOT_FREE_NON_NULL (FormatHelpString); > + FormatHelpString =3D NULL; >=20 > // > // Format revocation time part. > @@ -4032,13 +4060,18 @@ FormatHelpInfo ( > Time->Minute, > Time->Second > ); > - > + FormatHelpString =3D HiiGetString (PrivateData->HiiHandle, STRING_TO= KEN > (STR_SIGNATURE_DATA_HELP_FORMAT_TIME), NULL); > + if (FormatHelpString =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), > + FormatHelpString, > TimeString > ); > + SECUREBOOT_FREE_NON_NULL (FormatHelpString); > + FormatHelpString =3D NULL; > } >=20 > *StringId =3D HiiSetString (PrivateData->HiiHandle, 0, HelpInfoString,= NULL); > @@ -4046,6 +4079,8 @@ ON_EXIT: > SECUREBOOT_FREE_NON_NULL (DataString); > SECUREBOOT_FREE_NON_NULL (HelpInfoString); >=20 > + SECUREBOOT_FREE_NON_NULL (FormatTypeString); > + > return Status; > } >=20 > @@ -4076,6 +4111,7 @@ LoadSignatureData ( > EFI_IFR_GUID_LABEL *StartLabel; > EFI_IFR_GUID_LABEL *EndLabel; > EFI_STRING_ID HelpStringId; > + EFI_STRING FormatNameString; > VOID *StartOpCodeHandle; > VOID *EndOpCodeHandle; > UINTN DataSize; > @@ -4086,6 +4122,7 @@ LoadSignatureData ( > CHAR16 NameBuffer[BUFFER_MAX_SIZE]; >=20 > Status =3D EFI_SUCCESS; > + FormatNameString =3D NULL; > StartOpCodeHandle =3D NULL; > EndOpCodeHandle =3D NULL; > Index =3D 0; > @@ -4167,17 +4204,18 @@ LoadSignatureData ( > ListWalker =3D (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + ListWalk= er- > >SignatureListSize); > } >=20 > + FormatNameString =3D HiiGetString (PrivateData->HiiHandle, STRING_TOKE= N > (STR_SIGNATURE_DATA_NAME_FORMAT), NULL); > + if (FormatNameString =3D=3D NULL) { > + goto ON_EXIT; > + } > + > DataWalker =3D (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker + > sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize); > for (Index =3D 0; Index < SIGNATURE_DATA_COUNTS(ListWalker); Index =3D= Index > + 1) { > // > // Format name buffer. > // > ZeroMem (NameBuffer, sizeof (NameBuffer)); > - UnicodeSPrint (NameBuffer, > - sizeof (NameBuffer), > - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN > (STR_SIGNATURE_DATA_NAME_FORMAT), NULL), > - Index + 1 > - ); > + UnicodeSPrint (NameBuffer, sizeof (NameBuffer), FormatNameString, In= dex > + 1); >=20 > // > // Format help info buffer. > @@ -4221,6 +4259,7 @@ ON_EXIT: > SECUREBOOT_FREE_NON_OPCODE (EndOpCodeHandle); >=20 > SECUREBOOT_FREE_NON_NULL (VariableData); > + SECUREBOOT_FREE_NON_NULL (FormatNameString); >=20 > return Status; > } > -- > 2.13.2.windows.1