Re: [PATCH 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool

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

Star,

Thanks for the comments. It's a good suggestion. I didn't know ReallocatePool() can do that.

Thanks
Jian

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, November 03, 2017 5:14 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool
> 
> Some of the change can reuse ReallocatePool() to make the code more simpler.
> 
> For example, the change in FrontPageCustomizedUiSupport.c.
> 
>      Count++;
>      if (Count >= CurrentSize) {
>       DriverListPtr = AllocatePool ((Count + UI_HII_DRIVER_LIST_SIZE) * sizeof
> (UI_HII_DRIVER_INSTANCE));
>        ASSERT (DriverListPtr != NULL);
>       CopyMem (DriverListPtr, gHiiDriverList, CurrentSize * sizeof
> (UI_HII_DRIVER_INSTANCE));
>        FreePool (gHiiDriverList);
>        gHiiDriverList = DriverListPtr;
>        CurrentSize += UI_HII_DRIVER_LIST_SIZE;
> 
> Could be
> 
>      Count++;
>      if (Count >= CurrentSize) {
>       gHiiDriverList = ReallocatePool (
>                          CurrentSize * sizeof (UI_HII_DRIVER_INSTANCE),
>                          (CurrentSize + UI_HII_DRIVER_LIST_SIZE) * sizeof
> (UI_HII_DRIVER_INSTANCE),
>                          gHiiDriverList
>                          );
>       ASSERT (gHiiDriverList != NULL);
>        CurrentSize += UI_HII_DRIVER_LIST_SIZE;
> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Friday, November 3, 2017 12:58 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Bi,
> Dandan <dandan.bi@intel.com>
> Subject: [PATCH 1/3] MdeModulePkg: 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: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@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>
> ---
>  MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c  |  3 ++-
>  .../BootMaintenanceManagerCustomizedUiSupport.c                |  3 ++-
>  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c        |  4 +++-
>  MdeModulePkg/Library/UefiHiiLib/HiiLib.c                       | 10 ++++++----
>  .../Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c       |  3 ++-
>  MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c   |  3 ++-
>  6 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c
> b/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c
> index 1505ef9319..74d890441b 100644
> --- a/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c
> +++ b/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c
> @@ -639,8 +639,9 @@ UiListThirdPartyDrivers (
> 
>      Count++;
>      if (Count >= CurrentSize) {
> -      DriverListPtr = AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SIZE) *
> sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList);
> +      DriverListPtr = AllocatePool ((Count + UI_HII_DRIVER_LIST_SIZE) * sizeof
> (UI_HII_DRIVER_INSTANCE));
>        ASSERT (DriverListPtr != NULL);
> +      CopyMem (DriverListPtr, gHiiDriverList, CurrentSize * sizeof
> (UI_HII_DRIVER_INSTANCE));
>        FreePool (gHiiDriverList);
>        gHiiDriverList = DriverListPtr;
>        CurrentSize += UI_HII_DRIVER_LIST_SIZE;
> diff --git
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM
> anagerCustomizedUiSupport.c
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM
> anagerCustomizedUiSupport.c
> index b25bc67c06..512c340741 100644
> ---
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM
> anagerCustomizedUiSupport.c
> +++
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM
> anagerCustomizedUiSupport.c
> @@ -435,8 +435,9 @@ BmmListThirdPartyDrivers (
> 
>      Count++;
>      if (Count >= CurrentSize) {
> -      DriverListPtr = AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SIZE) *
> sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList);
> +      DriverListPtr = AllocatePool ((Count + UI_HII_DRIVER_LIST_SIZE) * sizeof
> (UI_HII_DRIVER_INSTANCE));
>        ASSERT (DriverListPtr != NULL);
> +      CopyMem (DriverListPtr, gHiiDriverList, CurrentSize * sizeof
> (UI_HII_DRIVER_INSTANCE));
>        FreePool (gHiiDriverList);
>        gHiiDriverList = DriverListPtr;
>        CurrentSize += UI_HII_DRIVER_LIST_SIZE;
> diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> index 23ae6c5392..8be3e53edf 100644
> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> @@ -240,7 +240,9 @@ AddIdToMacDeviceList (
>    } else {
>      mMacDeviceList.MaxListLen += MAX_MAC_ADDRESS_NODE_LIST_LEN;
>      if (mMacDeviceList.CurListLen != 0) {
> -      TempDeviceList = (MENU_INFO_ITEM *)AllocateCopyPool (sizeof
> (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen, (VOID
> *)mMacDeviceList.NodeList);
> +      TempDeviceList = (MENU_INFO_ITEM *)AllocatePool (sizeof
> (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen);
> +      ASSERT (TempDeviceList != NULL);
> +      CopyMem (TempDeviceList, mMacDeviceList.NodeList, sizeof
> (MENU_INFO_ITEM) * mMacDeviceList.CurListLen);
>      } else {
>        TempDeviceList = (MENU_INFO_ITEM *)AllocatePool (sizeof
> (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen);
>      }
> diff --git a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> index ce894c08b5..d88614fea5 100644
> --- a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> +++ b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> @@ -464,20 +464,22 @@ HiiGetFormSetFromHiiHandle(
>        }
> 
>        if (FormSetBuffer != NULL){
> -        TempBuffer = AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length, FormSetBuffer);
> -        FreePool(FormSetBuffer);
> -        FormSetBuffer = NULL;
> +        TempBuffer = AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length);
>          if (TempBuffer == NULL) {
>            Status = EFI_OUT_OF_RESOURCES;
>            goto Done;
>          }
> +        CopyMem (TempBuffer, FormSetBuffer, TempSize);
>          CopyMem (TempBuffer + TempSize,  OpCodeData, ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length);
> +        FreePool(FormSetBuffer);
> +        FormSetBuffer = NULL;
>        } else {
> -        TempBuffer = AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length, OpCodeData);
> +        TempBuffer = AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length);
>          if (TempBuffer == NULL) {
>            Status = EFI_OUT_OF_RESOURCES;
>            goto Done;
>          }
> +        CopyMem (TempBuffer, OpCodeData, ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length);
>        }
>        TempSize += ((EFI_IFR_OP_HEADER *) OpCodeData)->Length;
>        FormSetBuffer = TempBuffer;
> diff --git
> a/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c
> b/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c
> index b81110ff98..5bd4c246bc 100644
> --- a/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c
> +++ b/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c
> @@ -562,7 +562,8 @@ FvSimpleFileSystemOpen (
>        // No, there was no extension. So add one and search again for the file
>        // NewFileNameLength = FileNameLength + 1 + 4 = (Number of non-null
> character) + (file extension) + (a null character)
>        NewFileNameLength = FileNameLength + 1 + 4;
> -      FileNameWithExtension = AllocateCopyPool (NewFileNameLength * 2,
> FileName);
> +      FileNameWithExtension = AllocatePool (NewFileNameLength * 2);
> +      StrCpyS (FileNameWithExtension, FileNameLength, FileName);
>        StrCatS (FileNameWithExtension, NewFileNameLength, L".EFI");
> 
>        for (FvFileInfoLink = GetFirstNode (&Instance->FileInfoHead);
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> index 1b48c1cebe..47173b9c17 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> @@ -2543,10 +2543,11 @@ MergeToMultiKeywordResp (
> 
>    MultiKeywordRespLen = (StrLen (*MultiKeywordResp) + 1 + StrLen
> (*KeywordResp) + 1) * sizeof (CHAR16);
> 
> -  StringPtr = AllocateCopyPool (MultiKeywordRespLen, *MultiKeywordResp);
> +  StringPtr = AllocatePool (MultiKeywordRespLen);
>    if (StringPtr == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> +  CopyMem (StringPtr, *MultiKeywordResp, StrSize (*MultiKeywordResp));
> 
>    FreePool (*MultiKeywordResp);
>    *MultiKeywordResp = StringPtr;
> --
> 2.14.1.windows.1