Re: [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool

["Wang, Jian J" <jian.j.wang@intel.com>]

Jim,

RellocatePool() will free old buffer but AllocateCopyPool() will not. So not all cases in which they can be replaced for each other.

Thanks,
Jian

> -----Original Message-----
> From: Jim.Dailey@dell.com [mailto:Jim.Dailey@dell.com]
> Sent: Friday, November 03, 2017 7:44 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: RE: [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> 
> Isn't ReallocatePool is the correct function to use in these cases?
> For example:
> 
>     NewCommandLine1 = ReallocatePool(NewSize, StrSize(OriginalCommandLine),
> OriginalCommandLine;
> 
> Regards,
> Jim
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni,
> Ruiyu
> Sent: Friday, November 3, 2017 3:23 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [edk2] [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> 
> 2 comments below.
> 
> -----Original Message-----
> From: Wang, Jian J
> Sent: Friday, November 3, 2017 12:58 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Bi,
> Dandan <dandan.bi@intel.com>
> Subject: [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> 
> AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" bytes of
> memory from old "Buffer" to new allocated one. If "AllocationSize" is bigger
> than size of "Buffer", heap memory overflow occurs during copy.
> 
> The solution is to allocate pool first then copy the necessary bytes to new
> memory. This can avoid copying extra bytes from unknown memory range.
> 
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Bi Dandan <dandan.bi@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  ShellPkg/Application/Shell/Shell.c                                 | 4 +++-
>  ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 6
> ++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
> index 5471930ba1..24a04ca323 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1646,7 +1646,9 @@ ShellConvertVariables (
>    //
>    // now do the replacements...
>    //
> -  NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine);
> +  NewCommandLine1 = AllocatePool(NewSize);
> +  ASSERT (NewCommandLine1 != NULL);
> [Ray] 1. Please do not use assertion because there is NULL check in the below if-
> statement.
>     The rule in ShellPkg is avoid using assertion.
> 
> +  CopyMem (NewCommandLine1, OriginalCommandLine, StrSize
> (OriginalCommandLine));
>    NewCommandLine2 = AllocateZeroPool(NewSize);
>    ItemTemp        = AllocateZeroPool(ItemSize+(2*sizeof(CHAR16)));
>    if (NewCommandLine1 == NULL || NewCommandLine2 == NULL || ItemTemp
> == NULL) {
> diff --git
> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> index 1122c89b8b..5de62219b3 100644
> --- a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> +++ b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> @@ -143,10 +143,11 @@ UpdateOptionalData(
>      OriginalOptionDataSize += (*(UINT16*)(OriginalData + sizeof(UINT32)));
>      OriginalOptionDataSize -= OriginalSize;
>      NewSize = OriginalSize - OriginalOptionDataSize + DataSize;
> -    NewData = AllocateCopyPool(NewSize, OriginalData);
> +    NewData = AllocatePool(NewSize);
>      if (NewData == NULL) {
>        Status = EFI_OUT_OF_RESOURCES;
>      } else {
> +      CopyMem (NewData, OriginalData, OriginalSize - OriginalOptionDataSize);
>        CopyMem(NewData + OriginalSize - OriginalOptionDataSize, Data, DataSize);
>      }
>    }
> @@ -1120,11 +1121,12 @@ BcfgAddOpt(
>          // Now we know how many EFI_INPUT_KEY structs we need to attach to
> the end of the EFI_KEY_OPTION struct.
>          // Re-allocate with the added information.
>          //
> -        KeyOptionBuffer = AllocateCopyPool(sizeof(EFI_KEY_OPTION) +
> (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount),
> &NewKeyOption);
> +        KeyOptionBuffer = AllocatePool (sizeof(EFI_KEY_OPTION) +
> (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount));
>          if (KeyOptionBuffer == NULL) {
>            ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM),
> gShellBcfgHiiHandle, L"bcfg");
>            ShellStatus = SHELL_OUT_OF_RESOURCES;
>          }
> [Ray] 2. Should the above NULL check return?
> +        CopyMem (KeyOptionBuffer, &NewKeyOption, sizeof(EFI_KEY_OPTION));
>        }
>        for (LoopCounter = 0 ; ShellStatus == SHELL_SUCCESS && LoopCounter <
> NewKeyOption.KeyData.Options.InputKeyCount; LoopCounter++) {
>          //
> --
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel