From: "Wang, Jian J" <jian.j.wang@intel.com>
To: "Long, Qin" <qin.long@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ye, Ting" <ting.ye@intel.com>, "lersek@redhat.com" <lersek@redhat.com>
Subject: Re: [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free
Date: Wed, 1 Nov 2017 07:30:58 +0000 [thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624CA72FF@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <20171031083930.12800-3-qin.long@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@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.ye@intel.com>; lersek@redhat.com; Wang, Jian J
> <jian.j.wang@intel.com>; Long, Qin <qin.long@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 <lersek@redhat.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@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
prev parent reply other threads:[~2017-11-01 7:27 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-31 8:39 [PATCH 0/2] CryptoPkg/BaseCryptLib: Correct CRT realloc Wrapper Long Qin
2017-10-31 8:39 ` [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper Long Qin
2017-11-01 7:28 ` Wang, Jian J
2017-11-01 7:59 ` Long, Qin
2017-10-31 8:39 ` [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free Long Qin
2017-11-01 7:30 ` Wang, Jian J [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D827630B58408649ACB04F44C510003624CA72FF@SHSMSX103.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox