From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org 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 4113122344336 for ; Thu, 18 Jan 2018 21:25:10 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jan 2018 21:30:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,380,1511856000"; d="scan'208";a="196965995" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.19]) ([10.239.9.19]) by fmsmga006.fm.intel.com with ESMTP; 18 Jan 2018 21:30:31 -0800 To: Jian J Wang , edk2-devel@lists.01.org Cc: Eric Dong , Star Zeng References: <20180118073843.3676-1-jian.j.wang@intel.com> From: "Ni, Ruiyu" Message-ID: Date: Fri, 19 Jan 2018 13:30:30 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180118073843.3676-1-jian.j.wang@intel.com> 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: Fri, 19 Jan 2018 05:25:11 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 1/18/2018 3:38 PM, Jian J Wang wrote: > 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 > --- > 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 == EfiConventionalMemory) { > + OldStart = Start; > + OldPages = NumberOfPages; > + > AdjustMemoryF (&Start, &NumberOfPages); > if (NumberOfPages == 0) { > return EFI_SUCCESS; > } > + > + // > + // It's safe to unset Guard page inside memory lock because there should > + // 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/Page.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 == EfiConventionalMemory)) { > + // > + // 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 (Start == 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) (RangeEnd - 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 = Start; > - EndToClear = RangeEnd + 1; > - if (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT1|BIT0)) { > - if (IsGuardPage(StartToClear)) { > - StartToClear += EFI_PAGE_SIZE; > - } > - if (IsGuardPage (EndToClear - 1)) { > - EndToClear -= EFI_PAGE_SIZE; > - } > - } > - > - if (EndToClear > StartToClear) { > - DEBUG_CLEAR_MEMORY( > - (VOID *)(UINTN)StartToClear, > - (UINTN)(EndToClear - StartToClear) > - ); > - } > + DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd - Start + 1)); > } > } > > @@ -1513,9 +1488,6 @@ CoreInternalFreePages ( > > Done: > CoreReleaseMemoryLock (); > - if (IsGuarded) { > - UnsetGuardForMemory(Memory, NumberOfPages); > - } > return Status; > } > > diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c > index 7464d8773a..df9a1d28df 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c > @@ -643,10 +643,16 @@ CoreFreePoolPagesWithGuard ( > > AdjustMemoryF (&Memory, &NoPages); > if (NoPages > 0) { > + // > + // It's safe to unset Guard page inside memory lock because there should > + // 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); > } > > /** > Reviewed-by: Ruiyu Ni -- Thanks, Ray