From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 802212117D741 for ; Tue, 23 Oct 2018 09:41:42 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E8EEE91517; Tue, 23 Oct 2018 16:41:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-214.rdu2.redhat.com [10.10.120.214]) by smtp.corp.redhat.com (Postfix) with ESMTP id 53FFF1057040; Tue, 23 Oct 2018 16:41:40 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Michael D Kinney , Jiewen Yao , Ruiyu Ni , Star Zeng References: <20181023145331.5768-1-jian.j.wang@intel.com> <20181023145331.5768-3-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 23 Oct 2018 18:41:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181023145331.5768-3-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 23 Oct 2018 16:41:42 +0000 (UTC) Subject: Re: [PATCH v2 2/5] UefiCpuPkg/CpuDxe: fix an infinite loop issue X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Oct 2018 16:41:42 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/23/18 16:53, Jian J Wang wrote: >> v2 changes: >> a. Change the type of mPageTablePoolLock to be BOOLEAN. Related code >> logic is also updated and refined. >> b. Add non-stop mode for freed-memory guard feature > > The freed-memory guard feature will cause an infinite calling > of InitializePageTablePool(). (1) An important statement is missing from the commit message. Namely, under the UAF guard feature, FreePages() will modify page attributes. Similarly, the subject line is mostly useless. First, we're not "fixing" an issue: the issue doesn't exist yet. We're preventing the issue before the next patches could introduce it. Second, "infinite loop" is way too generic. Here's a suggestion: UefiCpuPkg/CpuDxe: prevent inf. recursion from FreePages->SetMemoryAttributes (77 characters. A bit longer than I'd like (which is 74), but given the long function names, it's hard to compress down the subject better. Also, I don't recommend "inf. loop", we have recursion here, not a plain loop.) > This is due to a fact that > AllocateAlignedPages() is used to allocate page table pool memory. > This function will most likely call gBS->FreePages to free unaligned > pages and then cause another round of page attributes change, like > below > > FreePages() <===============| > => SetMemoryAttributes() | > => | > => InitializePageTablePool() | > => AllocateAlignedPages() | > => FreePages() ================| > > The solution is add a global variable as a lock in page table pool > allocation function and fail any other requests if it has not been > done. (2) Please document what the practical consequences are when the global variable prevents the infinite recursion. As far as I can see, we are going to propagate various error codes as far as possible outwards. Questions: - Will this error reach the caller (for example, a 3rd party UEFI driver) if it checks the return status of gBS->FreePages()? - What is the consequence for the UAF guard? Is it only that the freed pages will not be marked inaccessible, or something more serious (like some inconsistency in internal platform firmware structures)? > This patch also add non-stop mode for freed-memory guard. (3) Please isolate the update to the non-stop macro (HEAP_GUARD_NONSTOP_MODE) to a separate patch. - Non-stop mode was introduced / explained in commit 8f2613628acf ("MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs", 2018-08-30). - The macro HEAP_GUARD_NONSTOP_MODE was added to CpuDxe in commit dcc026217fdc ("UefiCpuPkg/CpuDxe: implement non-stop mode for uefi", 2018-08-30). - Another instance of the macro HEAP_GUARD_NONSTOP_MODE was added to PiSmmCpuDxeSmm -- with BIT1|BIT0 replaced with BIT3|BIT2 -- in commit 09afd9a42a7f ("UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM", 2018-08-30) - According to the first patch in the present series, the UAF guard is UEFI-only. (Well, "DXE-only" would be more precise; the point is, it does not cover SMM.) Thus, the fact that we don't modify the macro definition in PiSmmCpuDxeSmm, and also *why* we don't do that, should be mentioned explicitly in the commit message of the patch that modifies HEAP_GUARD_NONSTOP_MODE in "UefiCpuPkg/CpuDxe/CpuDxe.h": UefiCpuPkg/CpuDxe: consider Use After Free guard in non-stop mode (65 characters) > Cc: Laszlo Ersek > Cc: Star Zeng > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > UefiCpuPkg/CpuDxe/CpuDxe.h | 2 +- > UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +++++++++++++++++-- > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h > index 064ea05bba..3183a3f7f4 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.h > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h > @@ -58,7 +58,7 @@ > ) > > #define HEAP_GUARD_NONSTOP_MODE \ > - ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6) > + ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6) > > #define NULL_DETECTION_NONSTOP_MODE \ > ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6) > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index 33e8ee2d2c..b7beaf935b 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > }; > > PAGE_TABLE_POOL *mPageTablePool = NULL; > +BOOLEAN mPageTablePoolLock = FALSE; > PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext; > EFI_SMM_BASE2_PROTOCOL *mSmmBase2 = NULL; > > @@ -1046,6 +1047,16 @@ InitializePageTablePool ( > VOID *Buffer; > BOOLEAN IsModified; > > + // > + // Do not allow re-entrance. > + // > + if (mPageTablePoolLock) { > + return FALSE; > + } > + > + mPageTablePoolLock = TRUE; > + IsModified = FALSE; > + > // > // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for > // header. > @@ -1056,7 +1067,9 @@ InitializePageTablePool ( > Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT); > if (Buffer == NULL) { > DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n")); > - return FALSE; > + goto Done; > + } else { > + DEBUG ((DEBUG_INFO, "Paging: added %ld pages to page table pool\r\n", (UINT64)PoolPages)); > } (4) Please don't add an "else" branch right after a "return" or "goto". (This is a very annoying, and regrettably frequent, anti-pattern in edk2.) (5) Please print UINT64 values with "%lu", not "%ld". (6) I think this message should be logged at the DEBUG_VERBOSE level. I don't insist, but please consider it at least. > // > @@ -1092,7 +1105,9 @@ InitializePageTablePool ( > ); > ASSERT (IsModified == TRUE); > > - return TRUE; > +Done: > + mPageTablePoolLock = FALSE; > + return IsModified; > } > > /** > Looks OK to me otherwise. Thanks, Laszlo