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 08E8C21A6F106 for ; Wed, 19 Apr 2017 00:26:51 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Apr 2017 00:26:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,220,1488873600"; d="scan'208";a="91592464" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 19 Apr 2017 00:26:51 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 19 Apr 2017 00:26:51 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 19 Apr 2017 00:26:50 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.246]) by SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0319.002; Wed, 19 Apr 2017 15:26:48 +0800 From: "Yao, Jiewen" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Wu, Hao" Thread-Topic: [PATCH] MdeModulePkg PiSmmCore: Enhance SMM FreePool to catch buffer overflow Thread-Index: AQHSuNM3CnLexDsHoEaBMd0l0zApWaHMSumw Date: Wed, 19 Apr 2017 07:26:47 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A92E685@shsmsx102.ccr.corp.intel.com> References: <1492582014-40100-1-git-send-email-star.zeng@intel.com> In-Reply-To: <1492582014-40100-1-git-send-email-star.zeng@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [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 07:26:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: jiewen.yao@intel.com > -----Original Message----- > From: Zeng, Star > Sent: Wednesday, April 19, 2017 2:07 PM > To: edk2-devel@lists.01.org > Cc: Zeng, Star ; Yao, Jiewen ;= Wu, > Hao > Subject: [PATCH] MdeModulePkg PiSmmCore: Enhance SMM FreePool to catch > buffer overflow >=20 > This solution is equivalent to DXE core. >=20 > 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(). >=20 > 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. >=20 > According to the PI spec (Vol 4, Section 3.2 SmmAllocatePool()): > The SmmAllocatePool() function ... All allocations are eight-byte aligned= . >=20 > 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(-) >=20 > 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) >=20 > +#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; >=20 > +#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; >=20 > @@ -154,18 +155,26 @@ InternalAllocPoolByIndex ( > } else { > Status =3D InternalAllocPoolByIndex (PoolType, PoolIndex + 1, &Hdr); > if (!EFI_ERROR (Status)) { > + Hdr->Header.Signature =3D 0; > Hdr->Header.Size >>=3D 1; > Hdr->Header.Available =3D TRUE; > - Hdr->Header.Type =3D PoolType; > + Hdr->Header.Type =3D 0; > + Tail =3D HEAD_TO_TAIL(&Hdr->Header); > + Tail->Signature =3D 0; > + Tail->Size =3D 0; > InsertHeadList (&mSmmPoolLists[SmmPoolType][PoolIndex], > &Hdr->Link); > Hdr =3D (FREE_POOL_HEADER*)((UINT8*)Hdr + Hdr->Header.Size); > } > } >=20 > if (!EFI_ERROR (Status)) { > + Hdr->Header.Signature =3D POOL_HEAD_SIGNATURE; > Hdr->Header.Size =3D MIN_POOL_SIZE << PoolIndex; > Hdr->Header.Available =3D FALSE; > Hdr->Header.Type =3D PoolType; > + Tail =3D HEAD_TO_TAIL(&Hdr->Header); > + Tail->Signature =3D POOL_TAIL_SIGNATURE; > + Tail->Size =3D Hdr->Header.Size; > } >=20 > *FreePoolHdr =3D Hdr; > @@ -187,6 +196,7 @@ InternalFreePoolByIndex ( > { > UINTN PoolIndex; > SMM_POOL_TYPE SmmPoolType; > + POOL_TAIL *PoolTail; >=20 > ASSERT ((FreePoolHdr->Header.Size & (FreePoolHdr->Header.Size - 1)) = =3D=3D 0); > ASSERT (((UINTN)FreePoolHdr & (FreePoolHdr->Header.Size - 1)) =3D=3D 0= ); > @@ -195,7 +205,12 @@ InternalFreePoolByIndex ( > SmmPoolType =3D > UefiMemoryTypeToSmmPoolType(FreePoolHdr->Header.Type); >=20 > PoolIndex =3D (UINTN) (HighBitSet32 ((UINT32)FreePoolHdr->Header.Size)= - > MIN_POOL_SHIFT); > + FreePoolHdr->Header.Signature =3D 0; > FreePoolHdr->Header.Available =3D TRUE; > + FreePoolHdr->Header.Type =3D 0; > + PoolTail =3D HEAD_TO_TAIL(&FreePoolHdr->Header); > + PoolTail->Signature =3D 0; > + PoolTail->Size =3D 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; > } >=20 > - Size +=3D sizeof (*PoolHdr); > + // > + // Adjust the size by the pool header & tail overhead > + // > + Size +=3D POOL_OVERHEAD; > if (Size > MAX_POOL_SIZE) { > Size =3D EFI_SIZE_TO_PAGES (Size); > Status =3D SmmInternalAllocatePages (AllocateAnyPages, PoolType, Siz= e, > &Address); > @@ -244,9 +263,13 @@ SmmInternalAllocatePool ( > } >=20 > PoolHdr =3D (POOL_HEADER*)(UINTN)Address; > + PoolHdr->Signature =3D POOL_HEAD_SIGNATURE; > PoolHdr->Size =3D EFI_PAGES_TO_SIZE (Size); > PoolHdr->Available =3D FALSE; > PoolHdr->Type =3D PoolType; > + PoolTail =3D HEAD_TO_TAIL(PoolHdr); > + PoolTail->Signature =3D POOL_TAIL_SIGNATURE; > + PoolTail->Size =3D PoolHdr->Size; > *Buffer =3D PoolHdr + 1; > return Status; > } > @@ -317,13 +340,30 @@ SmmInternalFreePool ( > ) > { > FREE_POOL_HEADER *FreePoolHdr; > + POOL_TAIL *PoolTail; >=20 > if (Buffer =3D=3D NULL) { > return EFI_INVALID_PARAMETER; > } >=20 > FreePoolHdr =3D (FREE_POOL_HEADER*)((POOL_HEADER*)Buffer - 1); > + ASSERT (FreePoolHdr->Header.Signature =3D=3D POOL_HEAD_SIGNATURE); > ASSERT (!FreePoolHdr->Header.Available); > + PoolTail =3D HEAD_TO_TAIL(&FreePoolHdr->Header); > + ASSERT (PoolTail->Signature =3D=3D POOL_TAIL_SIGNATURE); > + ASSERT (FreePoolHdr->Header.Size =3D=3D PoolTail->Size); > + > + if (FreePoolHdr->Header.Signature !=3D POOL_HEAD_SIGNATURE) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (PoolTail->Signature !=3D POOL_TAIL_SIGNATURE) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (FreePoolHdr->Header.Size !=3D PoolTail->Size) { > + return EFI_INVALID_PARAMETER; > + } >=20 > if (FreePoolHdr->Header.Size > MAX_POOL_SIZE) { > ASSERT (((UINTN)FreePoolHdr & EFI_PAGE_MASK) =3D=3D 0); > -- > 2.7.0.windows.1