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.115; helo=mga14.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 5751C203525FB for ; Wed, 1 Nov 2017 00:27:10 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Nov 2017 00:31:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,327,1505804400"; d="scan'208";a="170382130" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga006.fm.intel.com with ESMTP; 01 Nov 2017 00:31:02 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 1 Nov 2017 00:31:01 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Wed, 1 Nov 2017 15:30:59 +0800 From: "Wang, Jian J" To: "Long, Qin" , "edk2-devel@lists.01.org" CC: "Ye, Ting" , "lersek@redhat.com" Thread-Topic: [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free Thread-Index: AQHTUiP4B84uxX79YE+CMJvqYYilF6L/IliQ Date: Wed, 1 Nov 2017 07:30:58 +0000 Message-ID: References: <20171031083930.12800-1-qin.long@intel.com> <20171031083930.12800-3-qin.long@intel.com> In-Reply-To: <20171031083930.12800-3-qin.long@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzA1YTg1ZmQtZWYyOS00Y2E5LWI5ZGMtNGUwNGU0ZDU5ODdmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJON3hsM0toQkR4MzZpc01wWCtGdzlSRVUyT3BFV1g5QVVGOUVrcU1ONVZTYmJlMW1jblJMYklaN0ZaeThVY0NHIn0= 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 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free 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: Wed, 01 Nov 2017 07:27:10 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jian J Wang > -----Original Message----- > From: Long, Qin > Sent: Tuesday, October 31, 2017 4:40 PM > To: edk2-devel@lists.01.org > Cc: Ye, Ting ; lersek@redhat.com; Wang, Jian J > ; Long, Qin > Subject: [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory > allocation/free >=20 > The malloc/free (instead of AllocatePool/FreePool) were used directly > in some wrapper implementations, which was designed to leverage the > light-weight memory management routines at Runtime phase. > The malloc/free and AllocatePool/FreePool usages are required to be > matched, after extra memory size info header was introduced in malloc > wrapper. >=20 > This patch corrects two memory allocation cases, which requires the > caller to free the buffer with FreePool() outside the function call. >=20 > And some comments were also added to clarify the correct memory > release functions if it's the caller's responsibility to free the > memory buffer. >=20 > Cc: Laszlo Ersek > Cc: Ting Ye > Cc: Jian J Wang > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Qin Long > --- > CryptoPkg/Include/Library/BaseCryptLib.h | 16 ++++++++++= ------ > CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c | 5 +++-- > CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c | 3 ++- > CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c | 15 +++++++++-= ----- > CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c | 13 ++++++++--= --- > 5 files changed, 32 insertions(+), 20 deletions(-) >=20 > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > b/CryptoPkg/Include/Library/BaseCryptLib.h > index 5f67ecb709..e2b6a95666 100644 > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > @@ -2388,10 +2388,12 @@ Pkcs5HashPassword ( > @param[in] P7Data Pointer to the PKCS#7 message to verify. > @param[in] P7Length Length of the PKCS#7 message in bytes. > @param[out] CertStack Pointer to Signer's certificates retrieved fr= om P7Data. > - It's caller's responsibility to free the buff= er. > + It's caller's responsibility to free the buff= er with > + Pkcs7FreeSigners(). > @param[out] StackLength Length of signer's certificates in bytes. > @param[out] TrustedCert Pointer to a trusted certificate from Signer'= s > certificates. > - It's caller's responsibility to free the buff= er. > + It's caller's responsibility to free the buff= er with > + Pkcs7FreeSigners(). > @param[out] CertLength Length of the trusted certificate in bytes. >=20 > @retval TRUE The operation is finished successfully. > @@ -2433,10 +2435,11 @@ Pkcs7FreeSigners ( > @param[in] P7Data Pointer to the PKCS#7 message. > @param[in] P7Length Length of the PKCS#7 message in bytes. > @param[out] SignerChainCerts Pointer to the certificates list chained= to > signer's > - certificate. It's caller's responsibilit= y to free the buffer. > + certificate. It's caller's responsibilit= y to free the buffer > + with Pkcs7FreeSigners(). > @param[out] ChainLength Length of the chained certificates list = buffer in > bytes. > @param[out] UnchainCerts Pointer to the unchained certificates li= sts. It's > caller's > - responsibility to free the buffer. > + responsibility to free the buffer with P= kcs7FreeSigners(). > @param[out] UnchainLength Length of the unchained certificates lis= t buffer > in bytes. >=20 > @retval TRUE The operation is finished successfully. > @@ -2472,7 +2475,8 @@ Pkcs7GetCertificatesList ( > @param[in] OtherCerts Pointer to an optional additional set of = certificates > to > include in the PKCS#7 signedData (e.g. an= y intermediate > CAs in the chain). > - @param[out] SignedData Pointer to output PKCS#7 signedData. > + @param[out] SignedData Pointer to output PKCS#7 signedData. It's= caller's > + responsibility to free the buffer with Fr= eePool(). > @param[out] SignedDataSize Size of SignedData in bytes. >=20 > @retval TRUE PKCS#7 data signing succeeded. > @@ -2540,7 +2544,7 @@ Pkcs7Verify ( > @param[in] P7Data Pointer to the PKCS#7 signed data to process= . > @param[in] P7Length Length of the PKCS#7 signed data in bytes. > @param[out] Content Pointer to the extracted content from the PK= CS#7 > signedData. > - It's caller's responsibility to free the buf= fer. > + It's caller's responsibility to free the buf= fer with FreePool(). > @param[out] ContentSize The size of the extracted content in bytes. >=20 > @retval TRUE The P7Data was correctly formatted for proce= ssing. > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c > index d3b1a907aa..0f61d4b4ad 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c > @@ -34,7 +34,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, EITHER EXPRESS OR IMPLIED. > @param[in] OtherCerts Pointer to an optional additional set of = certificates > to > include in the PKCS#7 signedData (e.g. an= y intermediate > CAs in the chain). > - @param[out] SignedData Pointer to output PKCS#7 signedData. > + @param[out] SignedData Pointer to output PKCS#7 signedData. It's= caller's > + responsibility to free the buffer with Fr= eePool(). > @param[out] SignedDataSize Size of SignedData in bytes. >=20 > @retval TRUE PKCS#7 data signing succeeded. > @@ -167,7 +168,7 @@ Pkcs7Sign ( > // is totally 19 bytes. > // > *SignedDataSize =3D P7DataSize - 19; > - *SignedData =3D malloc (*SignedDataSize); > + *SignedData =3D AllocatePool (*SignedDataSize); > if (*SignedData =3D=3D NULL) { > OPENSSL_free (P7Data); > goto _Exit; > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c > index 539bb6b7d5..1ce7202d91 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c > @@ -33,7 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, EITHER EXPRESS OR IMPLIED. > @param[in] OtherCerts Pointer to an optional additional set of = certificates > to > include in the PKCS#7 signedData (e.g. an= y intermediate > CAs in the chain). > - @param[out] SignedData Pointer to output PKCS#7 signedData. > + @param[out] SignedData Pointer to output PKCS#7 signedData. It's= caller's > + responsibility to free the buffer with Fr= eePool(). > @param[out] SignedDataSize Size of SignedData in bytes. >=20 > @retval FALSE This interface is not supported. > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > index bf67a1f569..296df028b1 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > @@ -240,10 +240,12 @@ _Exit: > @param[in] P7Data Pointer to the PKCS#7 message to verify. > @param[in] P7Length Length of the PKCS#7 message in bytes. > @param[out] CertStack Pointer to Signer's certificates retrieved fr= om P7Data. > - It's caller's responsibility to free the buff= er. > + It's caller's responsibility to free the buff= er with > + Pkcs7FreeSigners(). > @param[out] StackLength Length of signer's certificates in bytes. > @param[out] TrustedCert Pointer to a trusted certificate from Signer'= s > certificates. > - It's caller's responsibility to free the buff= er. > + It's caller's responsibility to free the buff= er with > + Pkcs7FreeSigners(). > @param[out] CertLength Length of the trusted certificate in bytes. >=20 > @retval TRUE The operation is finished successfully. > @@ -438,10 +440,11 @@ Pkcs7FreeSigners ( > @param[in] P7Data Pointer to the PKCS#7 message. > @param[in] P7Length Length of the PKCS#7 message in bytes. > @param[out] SignerChainCerts Pointer to the certificates list chained= to > signer's > - certificate. It's caller's responsibilit= y to free the buffer. > + certificate. It's caller's responsibilit= y to free the buffer > + with Pkcs7FreeSigners(). > @param[out] ChainLength Length of the chained certificates list = buffer in > bytes. > @param[out] UnchainCerts Pointer to the unchained certificates li= sts. It's > caller's > - responsibility to free the buffer. > + responsibility to free the buffer with P= kcs7FreeSigners(). > @param[out] UnchainLength Length of the unchained certificates lis= t buffer > in bytes. >=20 > @retval TRUE The operation is finished successfully. > @@ -921,7 +924,7 @@ _Exit: > @param[in] P7Data Pointer to the PKCS#7 signed data to process= . > @param[in] P7Length Length of the PKCS#7 signed data in bytes. > @param[out] Content Pointer to the extracted content from the PK= CS#7 > signedData. > - It's caller's responsibility to free the buf= fer. > + It's caller's responsibility to free the buf= fer with FreePool(). > @param[out] ContentSize The size of the extracted content in bytes. >=20 > @retval TRUE The P7Data was correctly formatted for proce= ssing. > @@ -996,7 +999,7 @@ Pkcs7GetAttachedContent ( > OctStr =3D Pkcs7->d.sign->contents->d.data; > if ((OctStr->length > 0) && (OctStr->data !=3D NULL)) { > *ContentSize =3D OctStr->length; > - *Content =3D malloc (*ContentSize); > + *Content =3D AllocatePool (*ContentSize); > if (*Content =3D=3D NULL) { > *ContentSize =3D 0; > goto _Exit; > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c > index 06602ec535..d3e8ec89a7 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c > @@ -25,10 +25,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, EITHER EXPRESS OR IMPLIED. > @param[in] P7Data Pointer to the PKCS#7 message to verify. > @param[in] P7Length Length of the PKCS#7 message in bytes. > @param[out] CertStack Pointer to Signer's certificates retrieved fr= om P7Data. > - It's caller's responsibility to free the buff= er. > + It's caller's responsibility to free the buff= er with > + Pkcs7FreeSigners(). > @param[out] StackLength Length of signer's certificates in bytes. > @param[out] TrustedCert Pointer to a trusted certificate from Signer'= s > certificates. > - It's caller's responsibility to free the buff= er. > + It's caller's responsibility to free the buff= er with > + Pkcs7FreeSigners(). > @param[out] CertLength Length of the trusted certificate in bytes. >=20 > @retval FALSE This interface is not supported. > @@ -75,10 +77,11 @@ Pkcs7FreeSigners ( > @param[in] P7Data Pointer to the PKCS#7 message. > @param[in] P7Length Length of the PKCS#7 message in bytes. > @param[out] SignerChainCerts Pointer to the certificates list chained= to > signer's > - certificate. It's caller's responsibilit= y to free the buffer. > + certificate. It's caller's responsibilit= y to free the buffer > + with Pkcs7FreeSigners(). > @param[out] ChainLength Length of the chained certificates list = buffer in > bytes. > @param[out] UnchainCerts Pointer to the unchained certificates li= sts. It's > caller's > - responsibility to free the buffer. > + responsibility to free the buffer with P= kcs7FreeSigners(). > @param[out] UnchainLength Length of the unchained certificates lis= t buffer > in bytes. >=20 > @retval TRUE The operation is finished successfully. > @@ -142,7 +145,7 @@ Pkcs7Verify ( > @param[in] P7Data Pointer to the PKCS#7 signed data to process= . > @param[in] P7Length Length of the PKCS#7 signed data in bytes. > @param[out] Content Pointer to the extracted content from the PK= CS#7 > signedData. > - It's caller's responsibility to free the buf= fer. > + It's caller's responsibility to free the buf= fer with FreePool(). > @param[out] ContentSize The size of the extracted content in bytes. >=20 > @retval TRUE The P7Data was correctly formatted for proce= ssing. > -- > 2.14.1.windows.1