Reviewed-by: Jian J Wang <jian.j.w...@intel.com>
> -----Original Message-----
> From: Long, Qin
> Sent: Tuesday, October 31, 2017 4:40 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting...@intel.com>; ler...@redhat.com; Wang, Jian J
> <jian.j.w...@intel.com>; Long, Qin <qin.l...@intel.com>
> Subject: [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory
> allocation/free
>
> 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.
>
> This patch corrects two memory allocation cases, which requires the
> caller to free the buffer with FreePool() outside the function call.
>
> 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.
>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ting Ye <ting...@intel.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.l...@intel.com>
> ---
> 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(-)
>
> 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 from
> P7Data.
> - It's caller's responsibility to free the buffer.
> + It's caller's responsibility to free the buffer
> 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 buffer.
> + It's caller's responsibility to free the buffer
> with
> + Pkcs7FreeSigners().
> @param[out] CertLength Length of the trusted certificate in bytes.
>
> @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 responsibility to
> free the buffer.
> + certificate. It's caller's responsibility 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 lists.
> It's
> caller's
> - responsibility to free the buffer.
> + responsibility to free the buffer with
> Pkcs7FreeSigners().
> @param[out] UnchainLength Length of the unchained certificates list
> buffer
> in bytes.
>
> @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. any
> 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
> FreePool().
> @param[out] SignedDataSize Size of SignedData in bytes.
>
> @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 PKCS#7
> signedData.
> - It's caller's responsibility to free the buffer.
> + It's caller's responsibility to free the buffer
> with FreePool().
> @param[out] ContentSize The size of the extracted content in bytes.
>
> @retval TRUE The P7Data was correctly formatted for
> processing.
> 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. any
> 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
> FreePool().
> @param[out] SignedDataSize Size of SignedData in bytes.
>
> @retval TRUE PKCS#7 data signing succeeded.
> @@ -167,7 +168,7 @@ Pkcs7Sign (
> // is totally 19 bytes.
> //
> *SignedDataSize = P7DataSize - 19;
> - *SignedData = malloc (*SignedDataSize);
> + *SignedData = AllocatePool (*SignedDataSize);
> if (*SignedData == 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. any
> 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
> FreePool().
> @param[out] SignedDataSize Size of SignedData in bytes.
>
> @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 from
> P7Data.
> - It's caller's responsibility to free the buffer.
> + It's caller's responsibility to free the buffer
> 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 buffer.
> + It's caller's responsibility to free the buffer
> with
> + Pkcs7FreeSigners().
> @param[out] CertLength Length of the trusted certificate in bytes.
>
> @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 responsibility to
> free the buffer.
> + certificate. It's caller's responsibility 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 lists.
> It's
> caller's
> - responsibility to free the buffer.
> + responsibility to free the buffer with
> Pkcs7FreeSigners().
> @param[out] UnchainLength Length of the unchained certificates list
> buffer
> in bytes.
>
> @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 PKCS#7
> signedData.
> - It's caller's responsibility to free the buffer.
> + It's caller's responsibility to free the buffer
> with FreePool().
> @param[out] ContentSize The size of the extracted content in bytes.
>
> @retval TRUE The P7Data was correctly formatted for
> processing.
> @@ -996,7 +999,7 @@ Pkcs7GetAttachedContent (
> OctStr = Pkcs7->d.sign->contents->d.data;
> if ((OctStr->length > 0) && (OctStr->data != NULL)) {
> *ContentSize = OctStr->length;
> - *Content = malloc (*ContentSize);
> + *Content = AllocatePool (*ContentSize);
> if (*Content == NULL) {
> *ContentSize = 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 from
> P7Data.
> - It's caller's responsibility to free the buffer.
> + It's caller's responsibility to free the buffer
> 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 buffer.
> + It's caller's responsibility to free the buffer
> with
> + Pkcs7FreeSigners().
> @param[out] CertLength Length of the trusted certificate in bytes.
>
> @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 responsibility to
> free the buffer.
> + certificate. It's caller's responsibility 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 lists.
> It's
> caller's
> - responsibility to free the buffer.
> + responsibility to free the buffer with
> Pkcs7FreeSigners().
> @param[out] UnchainLength Length of the unchained certificates list
> buffer
> in bytes.
>
> @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 PKCS#7
> signedData.
> - It's caller's responsibility to free the buffer.
> + It's caller's responsibility to free the buffer
> with FreePool().
> @param[out] ContentSize The size of the extracted content in bytes.
>
> @retval TRUE The P7Data was correctly formatted for
> processing.
> --
> 2.14.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
