[edk2] [PATCH] SecurityPkg/SecureBootConfigDxe: Add check to avoid

chenc2 posted 1 patch 7 years, 2 months ago
Failed in applying to current master (apply log)
.../SecureBootConfigDxe/SecureBootConfigImpl.c     | 97 +++++++++++++++-------
1 file changed, 66 insertions(+), 31 deletions(-)
[edk2] [PATCH] SecurityPkg/SecureBootConfigDxe: Add check to avoid
Posted by chenc2 7 years, 2 months ago
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.)

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: chenc2 <chen.a.chen@intel.com>
---
 .../SecureBootConfigDxe/SecureBootConfigImpl.c     | 97 +++++++++++++++-------
 1 file changed, 66 insertions(+), 31 deletions(-)

diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index acb0dc0558..4ce5172701 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.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;
@@ -3705,6 +3708,12 @@ LoadSignatureList (
     goto ON_EXIT;
   }
 
+  FormatNameString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LIST_NAME_FORMAT), NULL);
+  FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LIST_HELP_FORMAT), NULL);
+  if (FormatNameString == NULL || FormatHelpString == NULL) {
+    goto ON_EXIT;
+  }
+
   RemainingSize = DataSize;
   ListWalker    = (EFI_SIGNATURE_LIST *)VariableData;
   while ((RemainingSize > 0) && (RemainingSize >= ListWalker->SignatureListSize)) {
@@ -3725,21 +3734,23 @@ LoadSignatureList (
     } else {
       ListType = STRING_TOKEN (STR_LIST_TYPE_UNKNOWN);
     }
+    FormatTypeString = HiiGetString (PrivateData->HiiHandle, ListType, NULL);
+    if (FormatTypeString == NULL) {
+      goto ON_EXIT;
+    }
 
     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, Index + 1);
 
     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 = NULL;
 
     HiiCreateGotoOpCode (
       StartOpCodeHandle,
@@ -3777,6 +3788,8 @@ ON_EXIT:
   SECUREBOOT_FREE_NON_OPCODE (EndGotoHandle);
 
   SECUREBOOT_FREE_NON_NULL (VariableData);
+  SECUREBOOT_FREE_NON_NULL (FormatNameString);
+  SECUREBOOT_FREE_NON_NULL (FormatHelpString);
 
   PrivateData->ListCount = Index;
 
@@ -3922,6 +3935,8 @@ FormatHelpInfo (
   EFI_STATUS      Status;
   EFI_TIME        *Time;
   EFI_STRING_ID   ListTypeId;
+  EFI_STRING      FormatHelpString;
+  EFI_STRING      FormatTypeString;
   UINTN           DataSize;
   UINTN           HelpInfoIndex;
   UINTN           TotalSize;
@@ -3969,6 +3984,11 @@ FormatHelpInfo (
     goto ON_EXIT;
   }
 
+  FormatTypeString = HiiGetString (PrivateData->HiiHandle, ListTypeId, NULL);
+  if (FormatTypeString == NULL) {
+    goto ON_EXIT;
+  }
+
   TotalSize = 1024;
   HelpInfoString = AllocateZeroPool (TotalSize);
   if (HelpInfoString == NULL) {
@@ -3981,40 +4001,45 @@ FormatHelpInfo (
   //
   ZeroMem (GuidString, sizeof (GuidString));
   GuidToString(&DataEntry->SignatureOwner, GuidString, BUFFER_MAX_SIZE);
+  FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL);
+  if (FormatHelpString == NULL) {
+    goto ON_EXIT;
+  }
   HelpInfoIndex += 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 = NULL;
 
   //
   // Format content part, it depends on the type of signature list, hash value or CN.
   //
   if (IsCert) {
     GetCommonNameFromX509 (ListEntry, DataEntry, &DataString);
-    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),
-                       DataSize,
-                       DataString
-                     );
+    FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_CN), NULL);
   } else {
     //
     //  Format hash value for each signature data entry.
     //
     ParseHashValue (ListEntry, DataEntry, &DataString);
-    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),
-                       DataSize,
-                       DataString
-                     );
+    FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL);
+  }
+  if (FormatHelpString == NULL) {
+    goto ON_EXIT;
   }
+  HelpInfoIndex += UnicodeSPrint (
+                     &HelpInfoString[HelpInfoIndex],
+                     TotalSize - sizeof (CHAR16) * HelpInfoIndex,
+                     FormatHelpString,
+                     FormatTypeString,
+                     DataSize,
+                     DataString
+                   );
+  SECUREBOOT_FREE_NON_NULL (FormatHelpString);
+  FormatHelpString = NULL;
 
   //
   // Format revocation time part.
@@ -4032,13 +4057,18 @@ FormatHelpInfo (
       Time->Minute,
       Time->Second
     );
-
+    FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_TIME), NULL);
+    if (FormatHelpString == 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 = NULL;
   }
 
   *StringId = HiiSetString (PrivateData->HiiHandle, 0, HelpInfoString, NULL);
@@ -4046,6 +4076,8 @@ ON_EXIT:
   SECUREBOOT_FREE_NON_NULL (DataString);
   SECUREBOOT_FREE_NON_NULL (HelpInfoString);
 
+  SECUREBOOT_FREE_NON_NULL (FormatTypeString);
+
   return Status;
 }
 
@@ -4076,6 +4108,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;
@@ -4167,17 +4200,18 @@ LoadSignatureData (
     ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + ListWalker->SignatureListSize);
   }
 
+  FormatNameString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_NAME_FORMAT), NULL);
+  if (FormatNameString == NULL) {
+    goto ON_EXIT;
+  }
+
   DataWalker = (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker + sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
   for (Index = 0; Index < SIGNATURE_DATA_COUNTS(ListWalker); Index = 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, Index + 1);
 
     //
     // Format help info buffer.
@@ -4221,6 +4255,7 @@ ON_EXIT:
   SECUREBOOT_FREE_NON_OPCODE (EndOpCodeHandle);
 
   SECUREBOOT_FREE_NON_NULL (VariableData);
+  SECUREBOOT_FREE_NON_NULL (FormatNameString);
 
   return Status;
 }
-- 
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/SecureBootConfigDxe: Add check to avoid
Posted by Laszlo Ersek 7 years, 2 months ago
Hi,

On 10/18/17 06:50, chenc2 wrote:
> 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.)
> 
> 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: chenc2 <chen.a.chen@intel.com>
> ---
>  .../SecureBootConfigDxe/SecureBootConfigImpl.c     | 97 +++++++++++++++-------
>  1 file changed, 66 insertions(+), 31 deletions(-)

The subject line of this patch appears truncated.

Thanks
Laszlo

> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
> index acb0dc0558..4ce5172701 100644
> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.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;
> @@ -3705,6 +3708,12 @@ LoadSignatureList (
>      goto ON_EXIT;
>    }
>  
> +  FormatNameString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LIST_NAME_FORMAT), NULL);
> +  FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_LIST_HELP_FORMAT), NULL);
> +  if (FormatNameString == NULL || FormatHelpString == NULL) {
> +    goto ON_EXIT;
> +  }
> +
>    RemainingSize = DataSize;
>    ListWalker    = (EFI_SIGNATURE_LIST *)VariableData;
>    while ((RemainingSize > 0) && (RemainingSize >= ListWalker->SignatureListSize)) {
> @@ -3725,21 +3734,23 @@ LoadSignatureList (
>      } else {
>        ListType = STRING_TOKEN (STR_LIST_TYPE_UNKNOWN);
>      }
> +    FormatTypeString = HiiGetString (PrivateData->HiiHandle, ListType, NULL);
> +    if (FormatTypeString == NULL) {
> +      goto ON_EXIT;
> +    }
>  
>      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, Index + 1);
>  
>      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 = NULL;
>  
>      HiiCreateGotoOpCode (
>        StartOpCodeHandle,
> @@ -3777,6 +3788,8 @@ ON_EXIT:
>    SECUREBOOT_FREE_NON_OPCODE (EndGotoHandle);
>  
>    SECUREBOOT_FREE_NON_NULL (VariableData);
> +  SECUREBOOT_FREE_NON_NULL (FormatNameString);
> +  SECUREBOOT_FREE_NON_NULL (FormatHelpString);
>  
>    PrivateData->ListCount = Index;
>  
> @@ -3922,6 +3935,8 @@ FormatHelpInfo (
>    EFI_STATUS      Status;
>    EFI_TIME        *Time;
>    EFI_STRING_ID   ListTypeId;
> +  EFI_STRING      FormatHelpString;
> +  EFI_STRING      FormatTypeString;
>    UINTN           DataSize;
>    UINTN           HelpInfoIndex;
>    UINTN           TotalSize;
> @@ -3969,6 +3984,11 @@ FormatHelpInfo (
>      goto ON_EXIT;
>    }
>  
> +  FormatTypeString = HiiGetString (PrivateData->HiiHandle, ListTypeId, NULL);
> +  if (FormatTypeString == NULL) {
> +    goto ON_EXIT;
> +  }
> +
>    TotalSize = 1024;
>    HelpInfoString = AllocateZeroPool (TotalSize);
>    if (HelpInfoString == NULL) {
> @@ -3981,40 +4001,45 @@ FormatHelpInfo (
>    //
>    ZeroMem (GuidString, sizeof (GuidString));
>    GuidToString(&DataEntry->SignatureOwner, GuidString, BUFFER_MAX_SIZE);
> +  FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_GUID), NULL);
> +  if (FormatHelpString == NULL) {
> +    goto ON_EXIT;
> +  }
>    HelpInfoIndex += 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 = NULL;
>  
>    //
>    // Format content part, it depends on the type of signature list, hash value or CN.
>    //
>    if (IsCert) {
>      GetCommonNameFromX509 (ListEntry, DataEntry, &DataString);
> -    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),
> -                       DataSize,
> -                       DataString
> -                     );
> +    FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_CN), NULL);
>    } else {
>      //
>      //  Format hash value for each signature data entry.
>      //
>      ParseHashValue (ListEntry, DataEntry, &DataString);
> -    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),
> -                       DataSize,
> -                       DataString
> -                     );
> +    FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_HASH), NULL);
> +  }
> +  if (FormatHelpString == NULL) {
> +    goto ON_EXIT;
>    }
> +  HelpInfoIndex += UnicodeSPrint (
> +                     &HelpInfoString[HelpInfoIndex],
> +                     TotalSize - sizeof (CHAR16) * HelpInfoIndex,
> +                     FormatHelpString,
> +                     FormatTypeString,
> +                     DataSize,
> +                     DataString
> +                   );
> +  SECUREBOOT_FREE_NON_NULL (FormatHelpString);
> +  FormatHelpString = NULL;
>  
>    //
>    // Format revocation time part.
> @@ -4032,13 +4057,18 @@ FormatHelpInfo (
>        Time->Minute,
>        Time->Second
>      );
> -
> +    FormatHelpString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_HELP_FORMAT_TIME), NULL);
> +    if (FormatHelpString == 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 = NULL;
>    }
>  
>    *StringId = HiiSetString (PrivateData->HiiHandle, 0, HelpInfoString, NULL);
> @@ -4046,6 +4076,8 @@ ON_EXIT:
>    SECUREBOOT_FREE_NON_NULL (DataString);
>    SECUREBOOT_FREE_NON_NULL (HelpInfoString);
>  
> +  SECUREBOOT_FREE_NON_NULL (FormatTypeString);
> +
>    return Status;
>  }
>  
> @@ -4076,6 +4108,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;
> @@ -4167,17 +4200,18 @@ LoadSignatureData (
>      ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + ListWalker->SignatureListSize);
>    }
>  
> +  FormatNameString = HiiGetString (PrivateData->HiiHandle, STRING_TOKEN (STR_SIGNATURE_DATA_NAME_FORMAT), NULL);
> +  if (FormatNameString == NULL) {
> +    goto ON_EXIT;
> +  }
> +
>    DataWalker = (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker + sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
>    for (Index = 0; Index < SIGNATURE_DATA_COUNTS(ListWalker); Index = 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, Index + 1);
>  
>      //
>      // Format help info buffer.
> @@ -4221,6 +4255,7 @@ ON_EXIT:
>    SECUREBOOT_FREE_NON_OPCODE (EndOpCodeHandle);
>  
>    SECUREBOOT_FREE_NON_NULL (VariableData);
> +  SECUREBOOT_FREE_NON_NULL (FormatNameString);
>  
>    return Status;
>  }
> 

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