From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 38F042034A8DB for ; Thu, 18 Jan 2018 01:40:46 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jan 2018 01:46:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,376,1511856000"; d="scan'208";a="11335219" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga008.fm.intel.com with ESMTP; 18 Jan 2018 01:46:07 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 18 Jan 2018 01:46:06 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 18 Jan 2018 01:46:06 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.189]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Thu, 18 Jan 2018 17:46:04 +0800 From: "Zeng, Star" To: "Wang, Jian J" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , "Dong, Eric" , "Zeng, Star" Thread-Topic: [PATCH] MdeModulePkg/Core: fix a logic hole in page free Thread-Index: AQHTkC9jCr9wRC/j9UKkZ527ielAOqN5YdXg Date: Thu, 18 Jan 2018 09:46:03 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9FE8D3@shsmsx102.ccr.corp.intel.com> References: <20180118073843.3676-1-jian.j.wang@intel.com> In-Reply-To: <20180118073843.3676-1-jian.j.wang@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/Core: fix a logic hole in page free X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Jan 2018 09:40:47 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Star Zeng Thanks, Star -----Original Message----- From: Wang, Jian J=20 Sent: Thursday, January 18, 2018 3:39 PM To: edk2-devel@lists.01.org Cc: Ni, Ruiyu ; Dong, Eric ; Zeng,= Star Subject: [PATCH] MdeModulePkg/Core: fix a logic hole in page free This hole will cause page fault randomly. The root cause is that Guard page= , which is just freed back to page pool but not yet cleared not- present at= tribute, will be allocated right away by internal function CoreFreeMemoryMa= pStack(). The solution to this issue is to clear the not-present attribute = for freed Guard page before doing any free operation, instead of after thos= e operation. The reason we didn't do this before is due to the fact that manipulating pa= ge attributes might cause memory allocation action which would cause a dead= lock inside a memory allocation/free operation. So we always set or unset = Guard page outside the memory lock. After a thorough analysis, we believe c= learing a Guard page will not cause memory allocation because memory we're = to manipulate was already manipulated before for sure. Therefore there should be no memory allocation occurring in this situation. Since we cleared Guard page not-present attribute before freeing instead of= after freeing, the debug code to clear freed memory can now be restored to= its original way (aka no checking and bypassing Guard page). Cc: Ruiyu Ni Cc: Eric Dong Cc: Star Zeng Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 15 +++++++++++++ MdeModulePkg/Core/Dxe/Mem/Page.c | 40 ++++++-------------------------= ---- MdeModulePkg/Core/Dxe/Mem/Pool.c | 10 +++++++-- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/= Mem/HeapGuard.c index 0f035043e1..92753c7269 100644 --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c @@ -1127,11 +1127,26 @@ CoreConvertPagesWithGuard ( IN EFI_MEMORY_TYPE NewType ) { + UINT64 OldStart; + UINTN OldPages; + if (NewType =3D=3D EfiConventionalMemory) { + OldStart =3D Start; + OldPages =3D NumberOfPages; + AdjustMemoryF (&Start, &NumberOfPages); if (NumberOfPages =3D=3D 0) { return EFI_SUCCESS; } + + // + // It's safe to unset Guard page inside memory lock because there shou= ld + // be no memory allocation occurred in updating memory page attribute = at + // this point. And unsetting Guard page before free will prevent Guard + // page just freed back to pool from being allocated right away before + // marking it usable (from non-present to present). + // + UnsetGuardForMemory (OldStart, OldPages); } else { AdjustMemoryA (&Start, &NumberOfPages); } diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/P= age.c index db32d0f940..8d5d03a6d9 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Page.c +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c @@ -900,42 +900,17 @@ CoreConvertPagesEx ( // CoreAddRange (MemType, Start, RangeEnd, Attribute); if (ChangingType && (MemType =3D=3D EfiConventionalMemory)) { + // + // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because th= is + // macro will ASSERT() if address is 0. Instead, CoreAddRange() gua= rantees + // that the page starting at address 0 is always filled with zeros. + // if (Start =3D=3D 0) { - // - // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because = this - // macro will ASSERT() if address is 0. Instead, CoreAddRange() - // guarantees that the page starting at address 0 is always filled - // with zeros. - // if (RangeEnd > EFI_PAGE_SIZE) { DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) EFI_PAGE_SIZE, (UINTN) (Rang= eEnd - EFI_PAGE_SIZE + 1)); } } else { - // - // If Heap Guard is enabled, the page at the top and/or bottom of - // this memory block to free might be inaccessible. Skipping them - // to avoid page fault exception. - // - UINT64 StartToClear; - UINT64 EndToClear; - - StartToClear =3D Start; - EndToClear =3D RangeEnd + 1; - if (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT1|BIT0)) { - if (IsGuardPage(StartToClear)) { - StartToClear +=3D EFI_PAGE_SIZE; - } - if (IsGuardPage (EndToClear - 1)) { - EndToClear -=3D EFI_PAGE_SIZE; - } - } - - if (EndToClear > StartToClear) { - DEBUG_CLEAR_MEMORY( - (VOID *)(UINTN)StartToClear, - (UINTN)(EndToClear - StartToClear) - ); - } + DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd -=20 + Start + 1)); } } =20 @@ -1513,9 +1488,6 @@ CoreInternalFreePages ( =20 Done: CoreReleaseMemoryLock (); - if (IsGuarded) { - UnsetGuardForMemory(Memory, NumberOfPages); - } return Status; } =20 diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/P= ool.c index 7464d8773a..df9a1d28df 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c @@ -643,10 +643,16 @@ CoreFreePoolPagesWithGuard ( =20 AdjustMemoryF (&Memory, &NoPages); if (NoPages > 0) { + // + // It's safe to unset Guard page inside memory lock because there shou= ld + // be no memory allocation occurred in updating memory page attribute = at + // this point. And unsetting Guard page before free will prevent Guard + // page just freed back to pool from being allocated right away before + // marking it usable (from non-present to present). + // + UnsetGuardForMemory (MemoryGuarded, NoPagesGuarded); CoreFreePoolPagesI (PoolType, Memory, NoPages); } - - UnsetGuardForMemory (MemoryGuarded, NoPagesGuarded); } =20 /** -- 2.15.1.windows.2