From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8A84521A6F106 for ; Tue, 18 Apr 2017 23:07:03 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Apr 2017 23:07:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,219,1488873600"; d="scan'208";a="1158100476" Received: from shwdeopenpsi068.ccr.corp.intel.com ([10.239.9.20]) by fmsmga002.fm.intel.com with ESMTP; 18 Apr 2017 23:07:00 -0700 From: Star Zeng To: edk2-devel@lists.01.org Cc: Star Zeng , Jiewen Yao , Hao Wu Date: Wed, 19 Apr 2017 14:06:54 +0800 Message-Id: <1492582014-40100-1-git-send-email-star.zeng@intel.com> X-Mailer: git-send-email 2.7.0.windows.1 Subject: [PATCH] MdeModulePkg PiSmmCore: Enhance SMM FreePool to catch buffer overflow X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Apr 2017 06:07:03 -0000 This solution is equivalent to DXE core. AllocatePool() allocates POOL_TAIL after the buffer. This POOL_TAIL is checked at FreePool(). If the there is buffer overflow, the issue can be caught at FreePool(). This patch could also handle the eight-byte aligned allocation requirement. The discussion related to the eight-byte aligned allocation requirement is at https://lists.01.org/pipermail/edk2-devel/2017-April/009995.html. According to the PI spec (Vol 4, Section 3.2 SmmAllocatePool()): The SmmAllocatePool() function ... All allocations are eight-byte aligned. Cc: Jiewen Yao Cc: Hao Wu Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Star Zeng --- MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 22 ++++++++++++++--- MdeModulePkg/Core/PiSmmCore/Pool.c | 44 +++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h index c12805a2dd96..b6f815c68da0 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h @@ -1196,12 +1196,28 @@ extern LIST_ENTRY mSmmMemoryMap; // #define MAX_POOL_INDEX (MAX_POOL_SHIFT - MIN_POOL_SHIFT + 1) +#define POOL_HEAD_SIGNATURE SIGNATURE_32('p','h','d','0') + typedef struct { - UINTN Size; - BOOLEAN Available; - EFI_MEMORY_TYPE Type; + UINT32 Signature; + BOOLEAN Available; + EFI_MEMORY_TYPE Type; + UINTN Size; } POOL_HEADER; +#define POOL_TAIL_SIGNATURE SIGNATURE_32('p','t','a','l') + +typedef struct { + UINT32 Signature; + UINT32 Reserved; + UINTN Size; +} POOL_TAIL; + +#define POOL_OVERHEAD (sizeof(POOL_HEADER) + sizeof(POOL_TAIL)) + +#define HEAD_TO_TAIL(a) \ + ((POOL_TAIL *) (((CHAR8 *) (a)) + (a)->Size - sizeof(POOL_TAIL))); + typedef struct { POOL_HEADER Header; LIST_ENTRY Link; diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c b/MdeModulePkg/Core/PiSmmCore/Pool.c index 43ce869d1e1a..ebb9f8c49ee9 100644 --- a/MdeModulePkg/Core/PiSmmCore/Pool.c +++ b/MdeModulePkg/Core/PiSmmCore/Pool.c @@ -133,6 +133,7 @@ InternalAllocPoolByIndex ( { EFI_STATUS Status; FREE_POOL_HEADER *Hdr; + POOL_TAIL *Tail; EFI_PHYSICAL_ADDRESS Address; SMM_POOL_TYPE SmmPoolType; @@ -154,18 +155,26 @@ InternalAllocPoolByIndex ( } else { Status = InternalAllocPoolByIndex (PoolType, PoolIndex + 1, &Hdr); if (!EFI_ERROR (Status)) { + Hdr->Header.Signature = 0; Hdr->Header.Size >>= 1; Hdr->Header.Available = TRUE; - Hdr->Header.Type = PoolType; + Hdr->Header.Type = 0; + Tail = HEAD_TO_TAIL(&Hdr->Header); + Tail->Signature = 0; + Tail->Size = 0; InsertHeadList (&mSmmPoolLists[SmmPoolType][PoolIndex], &Hdr->Link); Hdr = (FREE_POOL_HEADER*)((UINT8*)Hdr + Hdr->Header.Size); } } if (!EFI_ERROR (Status)) { + Hdr->Header.Signature = POOL_HEAD_SIGNATURE; Hdr->Header.Size = MIN_POOL_SIZE << PoolIndex; Hdr->Header.Available = FALSE; Hdr->Header.Type = PoolType; + Tail = HEAD_TO_TAIL(&Hdr->Header); + Tail->Signature = POOL_TAIL_SIGNATURE; + Tail->Size = Hdr->Header.Size; } *FreePoolHdr = Hdr; @@ -187,6 +196,7 @@ InternalFreePoolByIndex ( { UINTN PoolIndex; SMM_POOL_TYPE SmmPoolType; + POOL_TAIL *PoolTail; ASSERT ((FreePoolHdr->Header.Size & (FreePoolHdr->Header.Size - 1)) == 0); ASSERT (((UINTN)FreePoolHdr & (FreePoolHdr->Header.Size - 1)) == 0); @@ -195,7 +205,12 @@ InternalFreePoolByIndex ( SmmPoolType = UefiMemoryTypeToSmmPoolType(FreePoolHdr->Header.Type); PoolIndex = (UINTN) (HighBitSet32 ((UINT32)FreePoolHdr->Header.Size) - MIN_POOL_SHIFT); + FreePoolHdr->Header.Signature = 0; FreePoolHdr->Header.Available = TRUE; + FreePoolHdr->Header.Type = 0; + PoolTail = HEAD_TO_TAIL(&FreePoolHdr->Header); + PoolTail->Signature = 0; + PoolTail->Size = 0; ASSERT (PoolIndex < MAX_POOL_INDEX); InsertHeadList (&mSmmPoolLists[SmmPoolType][PoolIndex], &FreePoolHdr->Link); return EFI_SUCCESS; @@ -223,6 +238,7 @@ SmmInternalAllocatePool ( ) { POOL_HEADER *PoolHdr; + POOL_TAIL *PoolTail; FREE_POOL_HEADER *FreePoolHdr; EFI_STATUS Status; EFI_PHYSICAL_ADDRESS Address; @@ -235,7 +251,10 @@ SmmInternalAllocatePool ( return EFI_INVALID_PARAMETER; } - Size += sizeof (*PoolHdr); + // + // Adjust the size by the pool header & tail overhead + // + Size += POOL_OVERHEAD; if (Size > MAX_POOL_SIZE) { Size = EFI_SIZE_TO_PAGES (Size); Status = SmmInternalAllocatePages (AllocateAnyPages, PoolType, Size, &Address); @@ -244,9 +263,13 @@ SmmInternalAllocatePool ( } PoolHdr = (POOL_HEADER*)(UINTN)Address; + PoolHdr->Signature = POOL_HEAD_SIGNATURE; PoolHdr->Size = EFI_PAGES_TO_SIZE (Size); PoolHdr->Available = FALSE; PoolHdr->Type = PoolType; + PoolTail = HEAD_TO_TAIL(PoolHdr); + PoolTail->Signature = POOL_TAIL_SIGNATURE; + PoolTail->Size = PoolHdr->Size; *Buffer = PoolHdr + 1; return Status; } @@ -317,13 +340,30 @@ SmmInternalFreePool ( ) { FREE_POOL_HEADER *FreePoolHdr; + POOL_TAIL *PoolTail; if (Buffer == NULL) { return EFI_INVALID_PARAMETER; } FreePoolHdr = (FREE_POOL_HEADER*)((POOL_HEADER*)Buffer - 1); + ASSERT (FreePoolHdr->Header.Signature == POOL_HEAD_SIGNATURE); ASSERT (!FreePoolHdr->Header.Available); + PoolTail = HEAD_TO_TAIL(&FreePoolHdr->Header); + ASSERT (PoolTail->Signature == POOL_TAIL_SIGNATURE); + ASSERT (FreePoolHdr->Header.Size == PoolTail->Size); + + if (FreePoolHdr->Header.Signature != POOL_HEAD_SIGNATURE) { + return EFI_INVALID_PARAMETER; + } + + if (PoolTail->Signature != POOL_TAIL_SIGNATURE) { + return EFI_INVALID_PARAMETER; + } + + if (FreePoolHdr->Header.Size != PoolTail->Size) { + return EFI_INVALID_PARAMETER; + } if (FreePoolHdr->Header.Size > MAX_POOL_SIZE) { ASSERT (((UINTN)FreePoolHdr & EFI_PAGE_MASK) == 0); -- 2.7.0.windows.1