From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.9850.1593781911396023038 for ; Fri, 03 Jul 2020 06:11:51 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WTyJRChP; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593781910; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dUJCk4/4Eof4Z3LxCN5AGXXC1FFpiwSCM5z5aaQ2N3c=; b=WTyJRChPvI5IPgWJsMrFmPypPPImMedUuKj47JeQxZfd1Xm46F8NL9PrhjyY+8ru8fX3EP bRxECG0Pcknk5tbLV0kkjmVBhTYS6XgrdhK3kvO0qzHPUJjSFBpACphTlCx+Np1BpDyM5M 0Q3rm9UmKHwTy7Iha+ew6miYkpwSmtc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-313-n8Hv-LIvPC2f2ufCNsbDgA-1; Fri, 03 Jul 2020 09:11:46 -0400 X-MC-Unique: n8Hv-LIvPC2f2ufCNsbDgA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6AEC6107ACCA; Fri, 3 Jul 2020 13:11:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-238.ams2.redhat.com [10.36.114.238]) by smtp.corp.redhat.com (Postfix) with ESMTP id 115432DE79; Fri, 3 Jul 2020 13:11:43 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 9/9] UefiCpuPkg/CpuMpPei: Enable paging and set NP flag to avoid TOCTOU (CVE-2019-11098) To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20200702051525.1102-1-guomin.jiang@intel.com> <20200702051525.1102-10-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: <375f9f9d-4474-2c70-81e8-aecbce1e625a@redhat.com> Date: Fri, 3 Jul 2020 15:11:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200702051525.1102-10-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/02/20 07:15, Guomin Jiang wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614 > > To avoid the TOCTOU, enable paging and set Not Present flag so when > access any code in the flash range, it will trigger #NP exception. > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Signed-off-by: Guomin Jiang > --- > UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 3 +++ > UefiCpuPkg/CpuMpPei/CpuPaging.c | 17 +++++++++++++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf > index caead3ce34d4..fd50b55f06cb 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf > @@ -46,6 +46,9 @@ [LibraryClasses] > BaseMemoryLib > CpuLib > > +[Guids] > + gEdkiiMigratedFvInfoGuid ## SOMETIMES_CONSUMES ## HOB > + > [Ppis] > gEfiPeiMpServicesPpiGuid ## PRODUCES > gEfiSecPlatformInformationPpiGuid ## SOMETIMES_CONSUMES > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c > index d0cbebf70bbf..af4069b42cdb 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c > @@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > #include > #include > +#include > > #include "CpuMpPei.h" > > @@ -605,6 +606,8 @@ MemoryDiscoveredPpiNotifyCallback ( > EFI_STATUS Status; > BOOLEAN InitStackGuard; > BOOLEAN InterruptState; > + EDKII_MIGRATED_FV_INFO *MigratedFvInfo; > + EFI_PEI_HOB_POINTERS Hob; > > InterruptState = SaveAndDisableInterrupts (); > Status = MigrateGdt (); > @@ -617,9 +620,9 @@ MemoryDiscoveredPpiNotifyCallback ( > // the task switch (for the sake of stack switch). > // > InitStackGuard = FALSE; > - if (IsIa32PaeSupported () && PcdGetBool (PcdCpuStackGuard)) { > + if (IsIa32PaeSupported ()) { > EnablePaging (); > - InitStackGuard = TRUE; > + InitStackGuard = PcdGetBool (PcdCpuStackGuard); > } > > Status = InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServices); > @@ -629,6 +632,16 @@ MemoryDiscoveredPpiNotifyCallback ( > SetupStackGuardPage (); > } > > + Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid); > + while (Hob.Raw != NULL) { > + MigratedFvInfo = GET_GUID_HOB_DATA (Hob); > + ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase, MigratedFvInfo->FvLength, 0); > + > + Hob.Raw = GET_NEXT_HOB (Hob); > + Hob.Raw = GetNextGuidHob (&gEdkiiMigratedFvInfoGuid, Hob.Raw); > + } > + CpuFlushTlb (); > + > return Status; > } > > (1) This patch calls EnablePaging() even if no "gEdkiiMigratedFvInfoGuid" HOB exists. In other words, assume "PcdMigrateTemporaryRamFirmwareVolumes" is FALSE. - In that case, PeiCore() will not call EvacuateTempRam(). - Consequently, zero "gEdkiiMigratedFvInfoGuid" HOBs are going to be built. - Consequently, this patch will never call ConvertMemoryPageAttributes() Why do we call EnablePaging() then (assuming IsIa32PaeSupported() returns TRUE)? (2) Consider the opposite case. Assume IsIa32PaeSupported() returns FALSE. Further assume that at least one "gEdkiiMigratedFvInfoGuid" HOB exists. Then ConvertMemoryPageAttributes() will be called, without us ever calling EnablePaging(). That doesn't seem right. I suggest: BOOLEAN InitStackGuard; EFI_PEI_HOB_POINTERS Hob; InitStackGuard = FALSE; Hob.Raw = NULL; // // Both the "stack guard" feature and the "temp RAM evacuation" // feature depend on IA32 PAE support. // if (IsIa32PaeSupported ()) { InitStackGuard = PcdGetBool (PcdCpuStackGuard); Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid); } // // If either feature is active, then paging is required; enable it. // if (InitStackGuard || Hob.Raw != NULL) { EnablePaging (); } /* ... */ if (InitStackGuard) { SetupStackGuardPage (); } /* note: no need to call GetFirstGuidHob() again! */ while (Hob.Raw != NULL) { /* ... */ } CpuFlushTlb (); Thanks Laszlo