From: Laszlo Ersek <lersek@redhat.com>
To: "Wu, Hao A" <hao.a.wu@intel.com>,
"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 10:27:26 +0200 [thread overview]
Message-ID: <9035a4f2-b877-3c22-a5a5-dd448df8aaf1@redhat.com> (raw)
In-Reply-To: <B80AF82E9BFB8E4FBD8C89DA810C6A0931D10A4C@SHSMSX104.ccr.corp.intel.com>
On 10/10/17 04:46, Wu, Hao A wrote:
> Some comments below.
Given that a new version of this patch looks necessary, I'd like to
request an improved subject line / commit message:
(1) For the subject prefix, we should use:
SecurityPkg/SecureBootConfigDxe: ...
(2) It looks like the issue (= NULL pointer dereferencing) can occur
when the platform does not install the appropriate strings into the HII
database. All these strings are apparently identified by STR_SIGNATURE_*
tokens. So how about the following subject:
SecurityPkg/SecureBootConfigDxe: cope with lack of STR_SIGNATURE_* tokens
(This subject is 73 characters long.)
(3) Can we please capture some details about the issue in the commit
message? Namely, I thought it was not possible for these tokens to be
missing. Apparently they *can* be missing. Can we describe, briefly, how
and when that happens, in practice?
Thanks!
Laszlo
>> -----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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
prev parent reply other threads:[~2017-10-10 8:24 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
2017-10-10 8:27 ` Laszlo Ersek [this message]
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=9035a4f2-b877-3c22-a5a5-dd448df8aaf1@redhat.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