public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/2] CryptoPkg/BaseCryptLib: Correct CRT realloc Wrapper
@ 2017-10-31  8:39 Long Qin
  2017-10-31  8:39 ` [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper Long Qin
  2017-10-31  8:39 ` [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free Long Qin
  0 siblings, 2 replies; 6+ messages in thread
From: Long Qin @ 2017-10-31  8:39 UTC (permalink / raw)
  To: edk2-devel; +Cc: ting.ye, lersek, jian.j.wang

There is one long-standing problem in current CRT realloc wrapper
implementation, which will cause the obvious buffer overflow issue
when re-allocating memory block.
One BZ report: https://bugzilla.tianocore.org/show_bug.cgi?id=605

This patch series is to fix this buffer overflow issue by introducing
one extra header to record the memory buffer size information.
And extra comments were also added to clarify the memory release routines
if the caller is required to free the memory block outside the function.

Long Qin (2):
  CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper
  CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free

 CryptoPkg/Include/Library/BaseCryptLib.h           | 16 +++--
 CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c |  5 +-
 .../Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c   |  3 +-
 .../Library/BaseCryptLib/Pk/CryptPkcs7Verify.c     | 15 +++--
 .../Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c | 13 ++--
 .../BaseCryptLib/SysCall/BaseMemAllocation.c       | 72 +++++++++++++++++++---
 6 files changed, 97 insertions(+), 27 deletions(-)

-- 
2.14.1.windows.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper
  2017-10-31  8:39 [PATCH 0/2] CryptoPkg/BaseCryptLib: Correct CRT realloc Wrapper Long Qin
@ 2017-10-31  8:39 ` Long Qin
  2017-11-01  7:28   ` Wang, Jian J
  2017-10-31  8:39 ` [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free Long Qin
  1 sibling, 1 reply; 6+ messages in thread
From: Long Qin @ 2017-10-31  8:39 UTC (permalink / raw)
  To: edk2-devel; +Cc: ting.ye, lersek, jian.j.wang, Qin Long

There is one long-standing problem in CRT realloc wrapper, which will
cause the obvious buffer overflow issue when re-allocating one bigger
memory block:
    void *realloc (void *ptr, size_t size)
    {
      //
      // BUG: hardcode OldSize == size! We have no any knowledge about
      // memory size of original pointer ptr.
      //
      return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
    }
This patch introduces one extra header to record the memory buffer size
information when allocating memory block from malloc routine, and re-wrap
the realloc() and free() routines to remove this BUG.

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>
---
 .../BaseCryptLib/SysCall/BaseMemAllocation.c       | 72 +++++++++++++++++++---
 1 file changed, 65 insertions(+), 7 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
index f390e0d449..ed37a0ff39 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
@@ -16,6 +16,18 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <CrtLibSupport.h>
 #include <Library/MemoryAllocationLib.h>
 
+//
+// Extra header to record the memory buffer size from malloc routine.
+//
+#define CRYPTMEM_HEAD_SIGNATURE    SIGNATURE_32('c','m','h','d')
+typedef struct {
+  UINT32    Signature;
+  UINT32    Reserved;
+  UINTN     Size;
+} CRYPTMEM_HEAD;
+
+#define CRYPTMEM_OVERHEAD      sizeof(CRYPTMEM_HEAD)
+
 //
 // -- Memory-Allocation Routines --
 //
@@ -23,27 +35,73 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 /* Allocates memory blocks */
 void *malloc (size_t size)
 {
-  return AllocatePool ((UINTN) size);
+  CRYPTMEM_HEAD  *PoolHdr;
+  UINTN          NewSize;
+  VOID           *Data;
+
+  //
+  // Adjust the size by the buffer header overhead
+  //
+  NewSize = (UINTN)(size) + CRYPTMEM_OVERHEAD;
+
+  Data  = AllocatePool (NewSize);
+  if (Data != NULL) {
+    PoolHdr = (CRYPTMEM_HEAD *)Data;
+    //
+    // Record the memory brief information
+    //
+    PoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
+    PoolHdr->Size      = size;
+  }
+  return (VOID *)(PoolHdr + 1);
 }
 
 /* Reallocate memory blocks */
 void *realloc (void *ptr, size_t size)
 {
-  //
-  // BUG: hardcode OldSize == size! We have no any knowledge about
-  // memory size of original pointer ptr.
-  //
-  return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
+  CRYPTMEM_HEAD  *OldPoolHdr;
+  CRYPTMEM_HEAD  *NewPoolHdr;
+  UINTN          OldSize;
+  UINTN          NewSize;
+  VOID           *Data;
+
+  NewSize = (UINTN)size + CRYPTMEM_OVERHEAD;
+  Data = AllocatePool (NewSize);
+  if (Data != NULL) {
+    NewPoolHdr = (CRYPTMEM_HEAD *)Data;
+    NewPoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
+    NewPoolHdr->Size      = size;
+    if (ptr != NULL) {
+      //
+      // Retrieve the original size from the buffer header.
+      //
+      OldPoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
+      ASSERT (OldPoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
+      OldSize = OldPoolHdr->Size;
+
+      //
+      // Duplicate the buffer content.
+      //
+      CopyMem ((VOID *)(NewPoolHdr + 1), ptr, MIN (OldSize, size));
+      FreePool ((VOID *)OldPoolHdr);
+    }
+  }
+
+  return (VOID *)(NewPoolHdr + 1);
 }
 
 /* De-allocates or frees a memory block */
 void free (void *ptr)
 {
+  CRYPTMEM_HEAD  *PoolHdr;
+
   //
   // In Standard C, free() handles a null pointer argument transparently. This
   // is not true of FreePool() below, so protect it.
   //
   if (ptr != NULL) {
-    FreePool (ptr);
+    PoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
+    ASSERT (PoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
+    FreePool (PoolHdr);
   }
 }
-- 
2.14.1.windows.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free
  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-10-31  8:39 ` Long Qin
  2017-11-01  7:30   ` Wang, Jian J
  1 sibling, 1 reply; 6+ messages in thread
From: Long Qin @ 2017-10-31  8:39 UTC (permalink / raw)
  To: edk2-devel; +Cc: ting.ye, lersek, jian.j.wang, Qin Long

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper
  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
  0 siblings, 1 reply; 6+ messages in thread
From: Wang, Jian J @ 2017-11-01  7:28 UTC (permalink / raw)
  To: Long, Qin, edk2-devel@lists.01.org; +Cc: Ye, Ting, lersek@redhat.com

Hi Qin,

Thanks for fixing this issue. Please find my comments below.

Besides that, the patch has been passed the boot validation.

Validated-by: Jian J Wang <jian.j.wang@intel.com>

Thanks,
Jian

> -----Original Message-----
> From: Long, Qin
> Sent: Tuesday, October 31, 2017 4:39 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 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in
> realloc wrapper
> 
> There is one long-standing problem in CRT realloc wrapper, which will
> cause the obvious buffer overflow issue when re-allocating one bigger
> memory block:
>     void *realloc (void *ptr, size_t size)
>     {
>       //
>       // BUG: hardcode OldSize == size! We have no any knowledge about
>       // memory size of original pointer ptr.
>       //
>       return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
>     }
> This patch introduces one extra header to record the memory buffer size
> information when allocating memory block from malloc routine, and re-wrap
> the realloc() and free() routines to remove this BUG.
> 
> 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>
> ---
>  .../BaseCryptLib/SysCall/BaseMemAllocation.c       | 72 +++++++++++++++++++-
> --
>  1 file changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> index f390e0d449..ed37a0ff39 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> @@ -16,6 +16,18 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <CrtLibSupport.h>
>  #include <Library/MemoryAllocationLib.h>
> 
> +//
> +// Extra header to record the memory buffer size from malloc routine.
> +//
> +#define CRYPTMEM_HEAD_SIGNATURE    SIGNATURE_32('c','m','h','d')
> +typedef struct {
> +  UINT32    Signature;
> +  UINT32    Reserved;
> +  UINTN     Size;
> +} CRYPTMEM_HEAD;
> +
> +#define CRYPTMEM_OVERHEAD      sizeof(CRYPTMEM_HEAD)

Any consideration of the "Reserved" field, Padding? Alignment? Future extendibility?

> +
>  //
>  // -- Memory-Allocation Routines --
>  //
> @@ -23,27 +35,73 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  /* Allocates memory blocks */
>  void *malloc (size_t size)
>  {
> -  return AllocatePool ((UINTN) size);
> +  CRYPTMEM_HEAD  *PoolHdr;
> +  UINTN          NewSize;
> +  VOID           *Data;
> +
> +  //
> +  // Adjust the size by the buffer header overhead
> +  //
> +  NewSize = (UINTN)(size) + CRYPTMEM_OVERHEAD;
> +
> +  Data  = AllocatePool (NewSize);
> +  if (Data != NULL) {
> +    PoolHdr = (CRYPTMEM_HEAD *)Data;
> +    //
> +    // Record the memory brief information
> +    //
> +    PoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
> +    PoolHdr->Size      = size;
> +  }
> +  return (VOID *)(PoolHdr + 1);
>  }
> 

Although it's very rare, the logic of code above doesn't consider case of Data == NULL.
And above code might not pass GCC build because there's a chance that PoolHdr is not
initialized.

>  /* Reallocate memory blocks */
>  void *realloc (void *ptr, size_t size)
>  {
> -  //
> -  // BUG: hardcode OldSize == size! We have no any knowledge about
> -  // memory size of original pointer ptr.
> -  //
> -  return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
> +  CRYPTMEM_HEAD  *OldPoolHdr;
> +  CRYPTMEM_HEAD  *NewPoolHdr;
> +  UINTN          OldSize;
> +  UINTN          NewSize;
> +  VOID           *Data;
> +
> +  NewSize = (UINTN)size + CRYPTMEM_OVERHEAD;
> +  Data = AllocatePool (NewSize);
> +  if (Data != NULL) {
> +    NewPoolHdr = (CRYPTMEM_HEAD *)Data;
> +    NewPoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
> +    NewPoolHdr->Size      = size;
> +    if (ptr != NULL) {
> +      //
> +      // Retrieve the original size from the buffer header.
> +      //
> +      OldPoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
> +      ASSERT (OldPoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
> +      OldSize = OldPoolHdr->Size;
> +
> +      //
> +      // Duplicate the buffer content.
> +      //
> +      CopyMem ((VOID *)(NewPoolHdr + 1), ptr, MIN (OldSize, size));
> +      FreePool ((VOID *)OldPoolHdr);
> +    }
> +  }
> +
> +  return (VOID *)(NewPoolHdr + 1);
>  }
> 

1. The same as above, the code logic doesn't consider the case of Data == NULL.
2. ptr should be better checked against NULL before allocating new pool

>  /* De-allocates or frees a memory block */
>  void free (void *ptr)
>  {
> +  CRYPTMEM_HEAD  *PoolHdr;
> +
>    //
>    // In Standard C, free() handles a null pointer argument transparently. This
>    // is not true of FreePool() below, so protect it.
>    //
>    if (ptr != NULL) {
> -    FreePool (ptr);
> +    PoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
> +    ASSERT (PoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
> +    FreePool (PoolHdr);
>    }
>  }
> --
> 2.14.1.windows.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free
  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
  0 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-11-01  7:30 UTC (permalink / raw)
  To: Long, Qin, edk2-devel@lists.01.org; +Cc: Ye, Ting, lersek@redhat.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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper
  2017-11-01  7:28   ` Wang, Jian J
@ 2017-11-01  7:59     ` Long, Qin
  0 siblings, 0 replies; 6+ messages in thread
From: Long, Qin @ 2017-11-01  7:59 UTC (permalink / raw)
  To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Ye, Ting, lersek@redhat.com

Thanks, Jian. It's great to pass the validation. 
And exactly, the null data checking was missed. I will re-produce the V2 patch. 


Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Wang, Jian J 
Sent: Wednesday, November 1, 2017 3:28 PM
To: Long, Qin <qin.long@intel.com>; edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>; lersek@redhat.com
Subject: RE: [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper

Hi Qin,

Thanks for fixing this issue. Please find my comments below.

Besides that, the patch has been passed the boot validation.

Validated-by: Jian J Wang <jian.j.wang@intel.com>

Thanks,
Jian

> -----Original Message-----
> From: Long, Qin
> Sent: Tuesday, October 31, 2017 4:39 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 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue 
> in realloc wrapper
> 
> There is one long-standing problem in CRT realloc wrapper, which will 
> cause the obvious buffer overflow issue when re-allocating one bigger 
> memory block:
>     void *realloc (void *ptr, size_t size)
>     {
>       //
>       // BUG: hardcode OldSize == size! We have no any knowledge about
>       // memory size of original pointer ptr.
>       //
>       return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
>     }
> This patch introduces one extra header to record the memory buffer 
> size information when allocating memory block from malloc routine, and 
> re-wrap the realloc() and free() routines to remove this BUG.
> 
> 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>
> ---
>  .../BaseCryptLib/SysCall/BaseMemAllocation.c       | 72 +++++++++++++++++++-
> --
>  1 file changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git 
> a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> index f390e0d449..ed37a0ff39 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> @@ -16,6 +16,18 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> EITHER EXPRESS OR IMPLIED.
>  #include <CrtLibSupport.h>
>  #include <Library/MemoryAllocationLib.h>
> 
> +//
> +// Extra header to record the memory buffer size from malloc routine.
> +//
> +#define CRYPTMEM_HEAD_SIGNATURE    SIGNATURE_32('c','m','h','d')
> +typedef struct {
> +  UINT32    Signature;
> +  UINT32    Reserved;
> +  UINTN     Size;
> +} CRYPTMEM_HEAD;
> +
> +#define CRYPTMEM_OVERHEAD      sizeof(CRYPTMEM_HEAD)

Any consideration of the "Reserved" field, Padding? Alignment? Future extendibility?
[Long, Qin] There is no special consideration on this field. 
	Just keep this style as other POOL_HEAD usage, and may be for possible future extension. 

> +
>  //
>  // -- Memory-Allocation Routines --
>  //
> @@ -23,27 +35,73 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> EITHER EXPRESS OR IMPLIED.
>  /* Allocates memory blocks */
>  void *malloc (size_t size)
>  {
> -  return AllocatePool ((UINTN) size);
> +  CRYPTMEM_HEAD  *PoolHdr;
> +  UINTN          NewSize;
> +  VOID           *Data;
> +
> +  //
> +  // Adjust the size by the buffer header overhead  //  NewSize = 
> + (UINTN)(size) + CRYPTMEM_OVERHEAD;
> +
> +  Data  = AllocatePool (NewSize);
> +  if (Data != NULL) {
> +    PoolHdr = (CRYPTMEM_HEAD *)Data;
> +    //
> +    // Record the memory brief information
> +    //
> +    PoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
> +    PoolHdr->Size      = size;
> +  }
> +  return (VOID *)(PoolHdr + 1);
>  }
> 

Although it's very rare, the logic of code above doesn't consider case of Data == NULL.
And above code might not pass GCC build because there's a chance that PoolHdr is not initialized.

 [Long, Qin] Agree.


>  /* Reallocate memory blocks */
>  void *realloc (void *ptr, size_t size)  {
> -  //
> -  // BUG: hardcode OldSize == size! We have no any knowledge about
> -  // memory size of original pointer ptr.
> -  //
> -  return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
> +  CRYPTMEM_HEAD  *OldPoolHdr;
> +  CRYPTMEM_HEAD  *NewPoolHdr;
> +  UINTN          OldSize;
> +  UINTN          NewSize;
> +  VOID           *Data;
> +
> +  NewSize = (UINTN)size + CRYPTMEM_OVERHEAD;  Data = AllocatePool 
> + (NewSize);  if (Data != NULL) {
> +    NewPoolHdr = (CRYPTMEM_HEAD *)Data;
> +    NewPoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
> +    NewPoolHdr->Size      = size;
> +    if (ptr != NULL) {
> +      //
> +      // Retrieve the original size from the buffer header.
> +      //
> +      OldPoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
> +      ASSERT (OldPoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
> +      OldSize = OldPoolHdr->Size;
> +
> +      //
> +      // Duplicate the buffer content.
> +      //
> +      CopyMem ((VOID *)(NewPoolHdr + 1), ptr, MIN (OldSize, size));
> +      FreePool ((VOID *)OldPoolHdr);
> +    }
> +  }
> +
> +  return (VOID *)(NewPoolHdr + 1);
>  }
> 

1. The same as above, the code logic doesn't consider the case of Data == NULL.
2. ptr should be better checked against NULL before allocating new pool

 [Long, Qin] 1. Agree
	2. Looks it's not necessary. The logic is same as our ReallocatePool().

>  /* De-allocates or frees a memory block */  void free (void *ptr)  {
> +  CRYPTMEM_HEAD  *PoolHdr;
> +
>    //
>    // In Standard C, free() handles a null pointer argument transparently. This
>    // is not true of FreePool() below, so protect it.
>    //
>    if (ptr != NULL) {
> -    FreePool (ptr);
> +    PoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
> +    ASSERT (PoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
> +    FreePool (PoolHdr);
>    }
>  }
> --
> 2.14.1.windows.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-11-01  7:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox