From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 7AEEC211D2317 for ; Tue, 12 Jun 2018 01:04:51 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7874540201A0; Tue, 12 Jun 2018 08:04:50 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-15.rdu2.redhat.com [10.10.120.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id 28A662035721; Tue, 12 Jun 2018 08:04:49 +0000 (UTC) To: "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , "Yao, Jiewen" , "Dong, Eric" , "Zeng, Star" References: <20180611070833.5440-1-jian.j.wang@intel.com> <20180611070833.5440-2-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <9bd67c0c-027f-ab42-3aba-5f90291d306e@redhat.com> Date: Tue, 12 Jun 2018 10:04:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 12 Jun 2018 08:04:50 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 12 Jun 2018 08:04:50 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jun 2018 08:04:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 06/12/18 06:32, Wang, Jian J wrote: > Hi Laszlo, > > Thank you very much for such thorough review. I'd like to explain a bit in advance. > > Putting aside the specific coding issues in my patch, one thing is clear that SMM mode > has its own page table. CpuDxe should not touch it even if its public interface is called > via gBS services, because it has no knowledge of SMM internal things and it might > jeopardize SMM operation if it does. But current CpuDxe doesn't take this into account. Can you explain this in more detail? I mean, when CpuDxe protocol members are called outside of SMM, CpuDxe obviously can't mess with SMM page tables, because those are located in SMRAM, and non-SMM code cannot access SMRAM (at that point). So, I think you mean that, CpuDxe currently modifies whatever page tables are pointed-to by CR3, even if CpuDxe protocol members are (indirectly) invoked from code that runs in SMM. This results in CpuDxe messing with SMM page tables that were not meant for CpuDxe to modify. Is my understanding correct? > I think it should be fixed anyway, even there's no Heap Guard feature, OK, if my above summary is correct, then this makes sense to me. > since current > design allows SMM to touch memory owned by DXE but not vice versa. Right; it is valid for a traditional SMM driver to allocate and free both SMRAM and DXE memory in its entry point function. > The basic idea of this patch is, if we want to access DXE page table in SMM mode, we > cannot do this via CR3 register (it has been reloaded by SMM) directly but via a stored > value in a global. This is feasible because page table is just a chunk of normal memory. > All we need is an entry address pointer. When exiting SMM mode back to DXE, the > updated page table will take effect immediately once CR3 is restored to DXE version. > That means we can change DXE page table attributes even in SMM mode. Makes sense. > Please see my other responses below. OK: [snip] >> (2) Can you remind me how the SMM page tables map non-SMRAM memory? >> >> Because, I assume that, after a FreePages() libary API call, the freed >> memory should not be accessible to either SMM code or normal DXE code. >> This seems to imply that both sets of page tables should be modified. >> What am I missing? >> > > For both SMM and DXE, we just do 1:1 full memory mapping in paging but in > different page tables. As far as I know, there's no such rules that freed pages > should not be accessible. I think it's just implementation dependent. Let's assume we have a DXE page allocation, with the heap guard enabled. To my understanding, the allocated area is preceded and succeeded by 1 page on each end, and both of those pages are marked as inaccessible, so that we get a page fault on buffer overflow / underflow. Because traditional SMM drivers are allowed to work with DXE memory in their entry point functions, I *believe* (I'm not sure) that the guard pages should be marked as inaccessible in both the DXE page tables and in the SMM page tables; so that buffer overflow/underflow trap in both operating modes. Is my understanding correct? FWIW, I based my question in (2) on the above understading -- if the heap guard feature marks the guard pages inaccessible for both operating modes at allocation time, then whatever PTE-level "undo" is performed at gBS->FreePages() time, should likely also modify both sets of page tables. Of course, if the DXE heap guard modifies the PTEs for the guard pages only in the DXE page tables, at allocation time, then the SMM page tables are irrelevant at release time as well. [snip] >>> +BOOLEAN >>> +IsInSmm ( >>> + VOID >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + BOOLEAN InSmm; >>> + >>> + InSmm = FALSE; >>> + if (mSmmBase2 == NULL) { >>> + Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL, >>> + (VOID **)&mSmmBase2); >> >> (7) Indentation is incorrect. >> > > It'll be fixed. > >>> + if (EFI_ERROR (Status)) { >>> + mSmmBase2 = NULL; >>> + } >> >> (8) I think you can simply drop the EFI_ERROR (Status) branch; >> "mSmmBase2" will simply remain NULL. You didn't comment on this: do you agree, or is it necessary for some reason to re-assign NULL to mSmmBase2? (For example, a static analyzer or a compiler complains etc...) [snip] >>> @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected ( >>> VOID >>> ) >>> { >>> - return ((AsmReadCr0 () & BIT16) != 0); >>> + if (!IsInSmm ()) { >>> + return ((AsmReadCr0 () & BIT16) != 0); >>> + } >>> + return FALSE; >>> } >>> >>> /** >>> @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect ( >>> VOID >>> ) >>> { >>> - AsmWriteCr0 (AsmReadCr0() & ~BIT16); >>> + if (!IsInSmm ()) { >>> + AsmWriteCr0 (AsmReadCr0 () & ~BIT16); >>> + } >>> } >>> >>> /** >>> @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect ( >>> VOID >>> ) >>> { >>> - AsmWriteCr0 (AsmReadCr0() | BIT16); >>> + if (!IsInSmm ()) { >>> + AsmWriteCr0 (AsmReadCr0 () | BIT16); >>> + } >>> } >> >> (11) I don't understand these changes at all. Why are these functions >> no-ops when CpuDxe is invoked in Management Mode? >> > > CR0.BIT16 is used to protect current (active) page table. The SDM writes, Write Protect (bit 16 of CR0) — When set, inhibits supervisor-level procedures from writing into read-only pages; when clear, allows supervisor-level procedures to write into read-only pages (regardless of the U/S bit setting; see Section 4.1.3 and Section 4.6). This flag facilitates implementation of the copy-on-write method of creating a new process (forking) used by operating systems such as UNIX. First, I think we should use a macro for "write protect" that is more telling than just BIT16. We have "CR0_WP" in both - MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h - UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h so maybe we should move that to some IndustryStandard header. Should be a separate patch / BZ anyway. Second, from looking at the CpuDxe code, it seems like we map the DXE page tables r/o most of the time (in the DXE page tables themselves), regardless of various security PCDs. And the WP bit makes that mapping effective. Is that correct? In turn, are the DXE page tables mapped r/w in the SMM page tables? Because, if they are, then I think CR0.WP makes no difference in SMM, for accessing the DXE page tables. In that case, I agree we had better not touch CR0 when the CpuDxe code executes in SMM. > In CpuDxe, we should > not touch SMM page table if in SMM mode. Although it looks like there's no > problem to set this bit anyway because we just want to access DXE page table, > I don't want to risk to expose any potential issues here. So I think it may be safer > not to touch any SMM paging settings from non-SMM code. OK -- please add a *lot* of comments to the code about this. :) Thanks! Laszlo