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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.