From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Chen, Chen A" <chen.a.chen@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Zhang, Chao B" <chao.b.zhang@intel.com>
Subject: Re: [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
Date: Tue, 10 Oct 2017 02:46:42 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A0931D10A4C@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20171010020807.16452-1-chen.a.chen@intel.com>
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 NULL
> dereference.
>
> Cc: Zhang Chao <chao.b.zhang@intel.com>
> Cc: Wu Hao <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chen A Chen <chen.a.chen@intel.com>
> ---
> .../SecureBootConfigDxe/SecureBootConfigImpl.c | 80 +++++++++++++++----
> ---
> 1 file changed, 57 insertions(+), 23 deletions(-)
>
> 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;
>
> Status = EFI_SUCCESS;
> + EfiStringTemp1 = NULL;
> + EfiStringTemp2 = NULL;
> StartOpCodeHandle = NULL;
> EndOpCodeHandle = NULL;
> StartGotoHandle = NULL;
> @@ -3755,17 +3759,19 @@ LoadSignatureList (
> ListType = STRING_TOKEN (STR_LIST_TYPE_UNKNOWN);
> }
>
> - 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 = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_LIST_NAME_FORMAT), NULL);
> + if (EfiStringTemp1 == NULL) {
> + goto ON_EXIT;
> + }
> + UnicodeSPrint (NameBuffer, 100, EfiStringTemp1, Index + 1);
> + EfiStringTemp1 = 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 = AllocateZeroPool (100);
* NameBuffer = 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 = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_LIST_HELP_FORMAT), NULL);
> + EfiStringTemp2 = HiiGetString (PrivateData->HiiHandle, ListType, NULL);
> + if (EfiStringTemp1 == NULL || EfiStringTemp2 == NULL) {
> + goto ON_EXIT;
> + }
> + UnicodeSPrint (HelpBuffer, 100, EfiStringTemp1, EfiStringTemp2,
> SIGNATURE_DATA_COUNTS (ListWalker));
>
> 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;
>
> Status = EFI_SUCCESS;
> + EfiStringTemp1 = NULL;
> + EfiStringTemp2 = NULL;
> Time = NULL;
> HelpInfoIndex = 0;
> GuidString = NULL;
> @@ -4016,6 +4026,10 @@ FormatHelpInfo (
> goto ON_EXIT;
> }
>
> + EfiStringTemp1 = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL);
> + if (EfiStringTemp1 == NULL) {
> + goto ON_EXIT;
> + }
> //
> // Format GUID part.
> //
> @@ -4023,20 +4037,29 @@ FormatHelpInfo (
> HelpInfoIndex += UnicodeSPrint (
> &HelpInfoString[HelpInfoIndex],
> TotalSize - sizeof(CHAR16) * HelpInfoIndex,
> - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL),
> + EfiStringTemp1,
> GuidString
> );
> + EfiStringTemp1 = NULL;
>
> + EfiStringTemp2 = HiiGetString (PrivateData->HiiHandle, ListTypeId, NULL);
> + if (EfiStringTemp2 == 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 = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_HELP_FORMAT_CN), NULL);
> + if (EfiStringTemp1 == NULL) {
> + goto ON_EXIT;
> + }
> HelpInfoIndex += UnicodeSPrint(
> &HelpInfoString[HelpInfoIndex],
> TotalSize - sizeof(CHAR16) * HelpInfoIndex,
> - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (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 = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL);
> + if (EfiStringTemp1 == NULL) {
> + goto ON_EXIT;
> + }
> HelpInfoIndex += UnicodeSPrint (
> &HelpInfoString[HelpInfoIndex],
> TotalSize - sizeof(CHAR16) * HelpInfoIndex,
> - HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL),
> - HiiGetString (PrivateData->HiiHandle, ListTypeId, NULL),
> + EfiStringTemp1,
> + EfiStringTemp2,
> DataSize,
> DataString
> );
> }
> + EfiStringTemp1 = NULL;
>
> //
> // Format revocation time part.
> @@ -4077,10 +4105,14 @@ FormatHelpInfo (
> Time->Second
> );
>
> + EfiStringTemp1 = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_HELP_FORMAT_TIME), NULL);
> + if (EfiStringTemp1 == 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;
>
> Status = EFI_SUCCESS;
> + EfiString = NULL;
> StartOpCodeHandle = NULL;
> EndOpCodeHandle = NULL;
> Index = 0;
> @@ -4229,15 +4263,15 @@ LoadSignatureData (
> }
>
> DataWalker = (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker +
> sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
> + EfiString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN
> (STR_SIGNATURE_DATA_NAME_FORMAT), NULL);
> + if (EfiString == NULL) {
> + goto ON_EXIT;
> + }
> for (Index = 0; Index < SIGNATURE_DATA_COUNTS(ListWalker); Index = 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);
>
> //
> // Format help info buffer.
> --
> 2.13.2.windows.1
next prev parent reply other threads:[~2017-10-10 2:43 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-10 2:08 [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference chenc2
2017-10-10 2:46 ` Wu, Hao A [this message]
2017-10-10 8:27 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A0931D10A4C@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox