From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8A8252034CF7C for ; Sun, 5 Nov 2017 16:45:09 -0800 (PST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Nov 2017 16:49:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,350,1505804400"; d="scan'208";a="332431078" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 05 Nov 2017 16:49:06 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 5 Nov 2017 16:49:06 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 5 Nov 2017 16:49:06 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Mon, 6 Nov 2017 08:49:04 +0800 From: "Wang, Jian J" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Bi, Dandan" Thread-Topic: [PATCH 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool Thread-Index: AQHTVGBXINsGHW2d1EW+BLk+GCmcvKMCXZcQgAQoOTA= Date: Mon, 6 Nov 2017 00:49:04 +0000 Message-ID: References: <20171103045759.26508-1-jian.j.wang@intel.com> <20171103045759.26508-2-jian.j.wang@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9B1686@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9B1686@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjQ1NzZiNjctOGY1YS00Mzg0LWFlY2MtYjc0YTNjZWE2OTc2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJ4aUIrVHZxTW5td2J3bm1PbnpcL2VxVEpxSE9VK1lLcnBRMFNhbTE5eWxEVUlTcThTR1lnUU9LRG9qRGJpRzd3TCJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Nov 2017 00:45:09 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Star, Thanks for the comments. It's a good suggestion. I didn't know ReallocatePo= ol() can do that. Thanks Jian > -----Original Message----- > From: Zeng, Star > Sent: Friday, November 03, 2017 5:14 PM > To: Wang, Jian J ; edk2-devel@lists.01.org > Cc: Dong, Eric ; Bi, Dandan ; > Zeng, Star > Subject: RE: [PATCH 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool >=20 > Some of the change can reuse ReallocatePool() to make the code more simpl= er. >=20 > For example, the change in FrontPageCustomizedUiSupport.c. >=20 > Count++; > if (Count >=3D CurrentSize) { > DriverListPtr =3D AllocatePool ((Count + UI_HII_DRIVER_LIST_SIZE) *= sizeof > (UI_HII_DRIVER_INSTANCE)); > ASSERT (DriverListPtr !=3D NULL); > CopyMem (DriverListPtr, gHiiDriverList, CurrentSize * sizeof > (UI_HII_DRIVER_INSTANCE)); > FreePool (gHiiDriverList); > gHiiDriverList =3D DriverListPtr; > CurrentSize +=3D UI_HII_DRIVER_LIST_SIZE; >=20 > Could be >=20 > Count++; > if (Count >=3D CurrentSize) { > gHiiDriverList =3D ReallocatePool ( > CurrentSize * sizeof (UI_HII_DRIVER_INSTANCE), > (CurrentSize + UI_HII_DRIVER_LIST_SIZE) * sizeof > (UI_HII_DRIVER_INSTANCE), > gHiiDriverList > ); > ASSERT (gHiiDriverList !=3D NULL); > CurrentSize +=3D UI_HII_DRIVER_LIST_SIZE; >=20 > 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 ; Dong, Eric ; B= i, > Dandan > Subject: [PATCH 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool >=20 > AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" byte= s of > memory from old "Buffer" to new allocated one. If "AllocationSize" is big= ger > than size of "Buffer", heap memory overflow occurs during copy. >=20 > The solution is to allocate pool first then copy the necessary bytes to n= ew > memory. This can avoid copying extra bytes from unknown memory range. >=20 > Cc: Star Zeng > Cc: Eric Dong > Cc: Bi Dandan > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > 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(-) >=20 > 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 ( >=20 > Count++; > if (Count >=3D CurrentSize) { > - DriverListPtr =3D AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SI= ZE) * > sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList); > + DriverListPtr =3D AllocatePool ((Count + UI_HII_DRIVER_LIST_SIZE) = * sizeof > (UI_HII_DRIVER_INSTANCE)); > ASSERT (DriverListPtr !=3D NULL); > + CopyMem (DriverListPtr, gHiiDriverList, CurrentSize * sizeof > (UI_HII_DRIVER_INSTANCE)); > FreePool (gHiiDriverList); > gHiiDriverList =3D DriverListPtr; > CurrentSize +=3D 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 ( >=20 > Count++; > if (Count >=3D CurrentSize) { > - DriverListPtr =3D AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SI= ZE) * > sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList); > + DriverListPtr =3D AllocatePool ((Count + UI_HII_DRIVER_LIST_SIZE) = * sizeof > (UI_HII_DRIVER_INSTANCE)); > ASSERT (DriverListPtr !=3D NULL); > + CopyMem (DriverListPtr, gHiiDriverList, CurrentSize * sizeof > (UI_HII_DRIVER_INSTANCE)); > FreePool (gHiiDriverList); > gHiiDriverList =3D DriverListPtr; > CurrentSize +=3D 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 +=3D MAX_MAC_ADDRESS_NODE_LIST_LEN; > if (mMacDeviceList.CurListLen !=3D 0) { > - TempDeviceList =3D (MENU_INFO_ITEM *)AllocateCopyPool (sizeof > (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen, (VOID > *)mMacDeviceList.NodeList); > + TempDeviceList =3D (MENU_INFO_ITEM *)AllocatePool (sizeof > (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen); > + ASSERT (TempDeviceList !=3D NULL); > + CopyMem (TempDeviceList, mMacDeviceList.NodeList, sizeof > (MENU_INFO_ITEM) * mMacDeviceList.CurListLen); > } else { > TempDeviceList =3D (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( > } >=20 > if (FormSetBuffer !=3D NULL){ > - TempBuffer =3D AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER = *) > OpCodeData)->Length, FormSetBuffer); > - FreePool(FormSetBuffer); > - FormSetBuffer =3D NULL; > + TempBuffer =3D AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *) > OpCodeData)->Length); > if (TempBuffer =3D=3D NULL) { > Status =3D EFI_OUT_OF_RESOURCES; > goto Done; > } > + CopyMem (TempBuffer, FormSetBuffer, TempSize); > CopyMem (TempBuffer + TempSize, OpCodeData, ((EFI_IFR_OP_HEADER= *) > OpCodeData)->Length); > + FreePool(FormSetBuffer); > + FormSetBuffer =3D NULL; > } else { > - TempBuffer =3D AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER = *) > OpCodeData)->Length, OpCodeData); > + TempBuffer =3D AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *) > OpCodeData)->Length); > if (TempBuffer =3D=3D NULL) { > Status =3D EFI_OUT_OF_RESOURCES; > goto Done; > } > + CopyMem (TempBuffer, OpCodeData, ((EFI_IFR_OP_HEADER *) > OpCodeData)->Length); > } > TempSize +=3D ((EFI_IFR_OP_HEADER *) OpCodeData)->Length; > FormSetBuffer =3D 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 =3D FileNameLength + 1 + 4 =3D (Number of non= -null > character) + (file extension) + (a null character) > NewFileNameLength =3D FileNameLength + 1 + 4; > - FileNameWithExtension =3D AllocateCopyPool (NewFileNameLength * 2, > FileName); > + FileNameWithExtension =3D AllocatePool (NewFileNameLength * 2); > + StrCpyS (FileNameWithExtension, FileNameLength, FileName); > StrCatS (FileNameWithExtension, NewFileNameLength, L".EFI"); >=20 > for (FvFileInfoLink =3D 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 ( >=20 > MultiKeywordRespLen =3D (StrLen (*MultiKeywordResp) + 1 + StrLen > (*KeywordResp) + 1) * sizeof (CHAR16); >=20 > - StringPtr =3D AllocateCopyPool (MultiKeywordRespLen, *MultiKeywordResp= ); > + StringPtr =3D AllocatePool (MultiKeywordRespLen); > if (StringPtr =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } > + CopyMem (StringPtr, *MultiKeywordResp, StrSize (*MultiKeywordResp)); >=20 > FreePool (*MultiKeywordResp); > *MultiKeywordResp =3D StringPtr; > -- > 2.14.1.windows.1