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 4FB6221162CAB for ; Wed, 13 Jun 2018 12:54:39 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7B3A519545B; Wed, 13 Jun 2018 19:54:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-190.rdu2.redhat.com [10.10.122.190]) by smtp.corp.redhat.com (Postfix) with ESMTP id D7B0C11166ED; Wed, 13 Jun 2018 19:54:36 +0000 (UTC) To: Andrew Fish , Jian J Wang Cc: edk2-devel , Ruiyu Ni , Jiewen Yao , Eric Dong References: <20180613053501.4604-1-jian.j.wang@intel.com> <20180613053501.4604-2-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <73ca17c7-ca4d-6746-e79b-f279f4b3e1c9@redhat.com> Date: Wed, 13 Jun 2018 21:54:36 +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.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 13 Jun 2018 19:54:38 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 13 Jun 2018 19:54:38 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 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: Wed, 13 Jun 2018 19:54:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/13/18 17:14, Andrew Fish wrote: > > >> On Jun 12, 2018, at 10:35 PM, Jian J Wang wrote: >> >>> v2: >>> a. add more specific explanations in commit message >>> b. add more comments in code >>> c. remove redundant logic in IsInSmm() >>> d. fix a logic hole in GetCurrentPagingContext() >>> e. replace meanless constant macro with meaning ones >> >> The MdePkg/Library/SmmMemoryAllocationLib, used only by DXE_SMM_DRIVER, >> allows to free memory allocated in DXE (before EndOfDxe). This is done >> by checking the memory range and calling gBS services to do real >> operation if the memory to free is out of SMRAM. If some memory related >> features, like Heap Guard, are enabled, gBS interface will turn to >> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), provided by >> DXE driver UefiCpuPkg/CpuDxe, to change memory paging attributes. This >> means we have part of DXE code running in SMM mode in certain >> circumstances. >> >> Because page table in SMM mode is different from DXE mode and CpuDxe >> always uses current registers (CR0, CR3, etc.) to get memory paging >> attributes, it cannot get the correct attributes of DXE memory in SMM >> mode from SMM page table. This will cause incorrect memory manipulations, >> like fail the releasing of Guard pages if Heap Guard is enabled. >> >> The solution in this patch is to store the DXE page table information >> (e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe >> driver. If CpuDxe detects it's in SMM mode, it will use this global >> variable to access page table instead of current processor registers. >> This can avoid retrieving wrong DXE memory paging attributes and changing >> SMM page table attributes unexpectedly. >> > > Are there any security implications having SMM depend on attacker controlled data (DXE page tables)? This thought crossed my mind as well, and a part of the questions that I raised earlier are related. The important thing to consider here is the call chain; in other words, the circumstances under which we can reach CpuDxe code *at all* while the processor is in SMM. The call chain starts when an SMM driver (a traditional SMM driver I reckon) calls FreePool() or FreePages(), and the SmmMemoryAllocationLib instance determines that the SMM driver is freeing *DXE* pool (or pages) memory, and not SMRAM. In this case, SmmMemoryAllocationLib forwards the call out to gBS->FreePool() or gBS->FreePages(). And, it is *from there* that we gradually reach CpuDxe (the provider of the CpuArch protocol) to mess with DXE page table entries -- because the gBS->FreePool() and gBS->FreePages() services do that in edk2, regardless of SMM, when the heap guard is enabled. Obviously, gBS points into attacker controlled memory, so as soon as SmmMemoryAllocationLib calls gBS->anything, the calling SMM driver has been compromised immediately, way before we reach CpuDxe or anything else in DXE. So why isn't SmmMemoryAllocationLib already broken? Because, I think, a traditional SMM driver *must not* call FreePool() and FreePages(), for potentially releasing DXE memory, outside its entry point function anyway. That is, such FreePool / FreePages calls are permitted only under the exact same circumstances where the driver is allowed to utilize any other DXE protocols and services anyway; despite potentially executing in SMM. Once all DXE and SMM drivers have been dispatched and we reach BDS, SMRAM is locked down (by platform BDS), and SMM drivers aren't allowed to touch *anything* DXE any longer. In other words, it is safe to rely on CpuDxe global variables in SMM because we only do that when calling gBS->anything is still safe as well -- that is, before any 3rd party code gets a chance to run. In my previous review at http://mid.mail-archive.com/aada511c-bdb9-d833-caa5-bee56cc47d27@redhat.com this topic was the subject of my question (9a): > So basically what I'd like to see here is evidence that > > (9a) the IsInSmm() helper function is never called outside of the > entry point of an MM driver, due to the restrictions on > gBS->LocateProtocol() and mSmmBase2->InSmm() I didn't get a clear answer to that, but I managed to convince myself, with the above argument. If any of the new code added to CpuDxe is reached in SMM *after* 3rd party code was launched, then at least one SMM driver messed up (calling FreePool / FreePages on DXE memory at the wrong time), and then the vulnerability exists in that driver without the CpuDxe changes already. Thus, I believe this series does not introduce a vulnerability or new risks, it just adds more logic to the end of a call chain that is *already* - either broken (if an SMM driver calls FreePool / FreePages on DXE memory at the wrong time), - or safe (if all SMM drivers call FreePool / FreePages on DXE memory only in their entry point functions). More generally, to be honest: I find it terribly risky that SmmMemoryAllocationLib contains convenience code like this. In my opinion, SmmMemoryAllocationLib should never ever silently call out to gBS->anything; if it determines that the memory to release is outside of SMRAM, it should call CpuDeadLoop(). SMM drivers that want to release DXE memory should always call gBS services directly, not through MemoryAllocationLib APIs. Such a clear distinction would have also made the verification of this patch a lot easier -- the argument would have started with, "CpuDxe code is reached in SMM when SMM drivers call gBS->FreeXxx()". And the restrictions on gBS->FreeXxx() (namely, that they are restricted to the entry point function) make for a simpler argument. Thanks, Laszlo