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.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.d...@intel.com>; Bi, Dandan <dandan...@intel.com>;
> Zeng, Star <star.z...@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.z...@intel.com>; Dong, Eric <eric.d...@intel.com>; Bi,
> Dandan <dandan...@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.z...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Bi Dandan <dandan...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.w...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
