public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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



  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