From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 B7F9221A6F108 for ; Wed, 19 Apr 2017 04:23:53 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Apr 2017 04:23:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,221,1488873600"; d="scan'208";a="91301818" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga005.fm.intel.com with ESMTP; 19 Apr 2017 04:23:52 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 19 Apr 2017 04:23:52 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 19 Apr 2017 04:23:52 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.178]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.117]) with mapi id 14.03.0319.002; Wed, 19 Apr 2017 19:23:50 +0800 From: "Wu, Hao A" To: "Yao, Jiewen" , "Zeng, Star" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH] MdeModulePkg PiSmmCore: Enhance SMM FreePool to catch buffer overflow Thread-Index: AQHSuN5eFC/4+3OfZEKBBqrP/R6K1KHMjOpg Date: Wed, 19 Apr 2017 11:23:50 +0000 Message-ID: References: <1492582014-40100-1-git-send-email-star.zeng@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A92E685@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503A92E685@shsmsx102.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 11:23:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Hao Wu Best Regards, Hao Wu > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ya= o, > Jiewen > Sent: Wednesday, April 19, 2017 3:27 PM > To: Zeng, Star; edk2-devel@lists.01.org > Cc: Wu, Hao > Subject: Re: [edk2] [PATCH] MdeModulePkg PiSmmCore: Enhance SMM > FreePool to catch buffer overflow >=20 > Reviewed-by: jiewen.yao@intel.com >=20 > > -----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 > > > > 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 align= ed. > > > > 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 =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); > > } > > } > > > > 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; > > } > > > > *FreePoolHdr =3D Hdr; > > @@ -187,6 +196,7 @@ InternalFreePoolByIndex ( > > { > > UINTN PoolIndex; > > SMM_POOL_TYPE SmmPoolType; > > + POOL_TAIL *PoolTail; > > > > 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); > > > > PoolIndex =3D (UINTN) (HighBitSet32 ((UINT32)FreePoolHdr->Header.Siz= e) - > > 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; > > } > > > > - 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, S= ize, > > &Address); > > @@ -244,9 +263,13 @@ SmmInternalAllocatePool ( > > } > > > > 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; > > > > if (Buffer =3D=3D NULL) { > > return EFI_INVALID_PARAMETER; > > } > > > > 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; > > + } > > > > if (FreePoolHdr->Header.Size > MAX_POOL_SIZE) { > > ASSERT (((UINTN)FreePoolHdr & EFI_PAGE_MASK) =3D=3D 0); > > -- > > 2.7.0.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel