1. Patchew
  2. EDK2
  3. View series

[edk2] [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.

chenc2 posted 1 patch 7 years, 2 months ago
Failed in applying to current master (apply log)
.../SecureBootConfigDxe/SecureBootConfigImpl.c     | 80 +++++++++++++++-------
1 file changed, 57 insertions(+), 23 deletions(-)
[edk2] [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
Posted by chenc2 7 years, 2 months ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
Posted by Wu, Hao A 7 years, 2 months ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] SecurityPkg/SecureBootConfigImpl.c: Fix the potential NULL dereference.
Posted by Laszlo Ersek 7 years, 2 months ago 
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
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.