From nobody Mon Dec 23 13:26:04 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) smtp.mailfrom=edk2-devel-bounces@lists.01.org Return-Path: Received: from ml01.01.org (ml01.01.org [198.145.21.10]) by mx.zohomail.com with SMTPS id 1516261131921242.05633215344528; Wed, 17 Jan 2018 23:38:51 -0800 (PST) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id ABF88221F93B6; Wed, 17 Jan 2018 23:33:27 -0800 (PST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 47CA52034A8DB for ; Wed, 17 Jan 2018 23:33:25 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jan 2018 23:38:46 -0800 Received: from jwang36-mobl2.ccr.corp.intel.com ([10.239.193.116]) by orsmga002.jf.intel.com with ESMTP; 17 Jan 2018 23:38:45 -0800 X-Original-To: edk2-devel@lists.01.org Received-SPF: none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) client-ip=198.145.21.10; envelope-from=edk2-devel-bounces@lists.01.org; helo=ml01.01.org; Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,376,1511856000"; d="scan'208";a="27419888" From: Jian J Wang To: edk2-devel@lists.01.org Date: Thu, 18 Jan 2018 15:38:43 +0800 Message-Id: <20180118073843.3676-1-jian.j.wang@intel.com> X-Mailer: git-send-email 2.15.1.windows.2 Subject: [edk2] [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: , Cc: Ruiyu Ni , Eric Dong , Star Zeng MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" X-ZohoMail: RSF_4 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" 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 attribute, will be allocated right away by internal function CoreFreeMemoryMapStack(). The solution to this issue is to clear the not-present attribute for freed Guard page before doing any free operation, instead of after those operation. The reason we didn't do this before is due to the fact that manipulating page 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 clearing 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 Reviewed-by: Ruiyu Ni Reviewed-by: Star Zeng --- 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 - Sta= rt + 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 /** --=20 2.15.1.windows.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel