Cryptlib: Track allocation sizes to fix realloc

[nicholasbishop]

Implementing realloc properly requires knowing the size of the allocation, so that the memory in the old allocation can be copied over to the new allocation. This information isn't provided by UEFI or gnu-efi. Previously the implementation of realloc copied over the new allocation size's number of bytes, which can fail with stricter memory protections.

Fix by expanding each allocation by 8 bytes, and storing the size of the allocation in the first 8 bytes of the allocation. The pointer returned by malloc is then 8 bytes after the actual pool allocation. The realloc and free functions subtract 8 bytes to get the "real" allocation pointer again for passing to the ReallocatePool and FreePool functions. The realloc function reads the original allocation size from the beginning of the allocation so that the correct number of bytes are copied to the new allocation.

Fixes https://github.com/rhboot/shim/issues/538

Signed-off-by: Nicholas Bishop nicholasbishop@google.com

[dennis-tseng99]

I merged malloc(), realloc() and free() from https://github.com/tianocore/edk2/blob/master/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c into shim in my local site, and did a testing roughly. It seems working well. Maybe we should align these 3 functions to EDK2 codes.

Here is my understanding about realloc() in the EDK2 codes:

[OldPoolHdr]

<--- CRYPTMEM_OVERHEAD ---><---- OldSize ------>
+-------------------------+--------------------+
|     PoolHdr             | real ptr   ......  |             
+-------------------------+--------------------+
^                         ^
OldPoolHdr               ptr

OldPoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
OldPoolHdr will be freed by FreePool()

[NewPoolHdr]

<-------------------------- NewSize ---------------------------------->
<--- CRYPTMEM_OVERHEAD ---><------------------ size ------------------>
+-------------------------+-------------------------------------------+
|        NewPoolHdr       |             empty space                   |             
+-------------------------+-------------------------------------------+
^                         ^
NewPoolHdr                NewPoolHdr + 1 <--- return vaule

Here is the orginal EDK2 committed patch:

Author: Long Qin <qin.long@intel.com>
Date:   Wed Nov 1 16:10:04 2017 +0800

    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>
    Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
    Validated-by: Jian J Wang <jian.j.wang@intel.com>

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c b/CryptoPkg/
Library/BaseCryptLib/SysCall/BaseMemAllocation.c
index f390e0d449..19c071e2bf 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,84 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRES
S 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);
+  } else {
+    //
+    // The buffer allocation failed.
+    //
+    return NULL;
+  }
 }

 /* 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);
+  } else {
+    //
+    // The buffer allocation failed.
+    //
+    return NULL;
+  }
 }

 /* 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);
   }
 }```

[nicholasbishop]

Thanks @dennis-tseng99, good find. Importing those changes from edk2 is probably the better path here, so I'll close this PR.