* [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
@ 2017-10-10 2:08 chenc2
2017-10-10 2:46 ` Wu, Hao A
0 siblings, 1 reply; 3+ messages in thread
From: chenc2 @ 2017-10-10 2:08 UTC (permalink / raw)
To: edk2-devel; +Cc: chenc2, Zhang Chao, Wu Hao
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/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index 3e03be9738..457e020ece 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
@@ -3579,7 +3579,9 @@ LoadSignatureList (
)
{
EFI_STATUS Status;
- EFI_STRING_ID ListType;
+ EFI_STRING EfiStringTemp1;
+ EFI_STRING EfiStringTemp2;
+ EFI_STRING_ID ListType;;
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;
+
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
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
0 siblings, 1 reply; 3+ messages in thread
From: Wu, Hao A @ 2017-10-10 2:46 UTC (permalink / raw)
To: Chen, Chen A, edk2-devel@lists.01.org; +Cc: Zhang, Chao B
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
2017-10-10 2:46 ` Wu, Hao A
@ 2017-10-10 8:27 ` Laszlo Ersek
0 siblings, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2017-10-10 8:27 UTC (permalink / raw)
To: Wu, Hao A, Chen, Chen A, edk2-devel@lists.01.org; +Cc: Zhang, Chao B
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-10 8:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox