From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.8428.1593776182760054201 for ; Fri, 03 Jul 2020 04:36:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=euqzLLPd; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593776182; 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=vUeot70v4q0RMlueRuD89+4gs3W1hgeIS7gv6BxNmGU=; b=euqzLLPdxlOzwI3PNMSJJ7jhT/2dtgvTTijEMh3EEFz8kLDDGiZLCTB5tcg20GrSz+1nR8 4v7wAibywgEuqrcSBXIoIxemaSuTBUNocbtbk90Qo/rphmIHS3Zll3WyAjxjmT5LqbVUfp eX4iIRZTcKN/sfIWqtDUZFnBCCxtLLo= 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-16-Aeuys9KqMZGzHd85cPgxkQ-1; Fri, 03 Jul 2020 07:36:15 -0400 X-MC-Unique: Aeuys9KqMZGzHd85cPgxkQ-1 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id 16B3A1940920; Fri, 3 Jul 2020 11:36:14 +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 7D8DE10013D0; Fri, 3 Jul 2020 11:36:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098) To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Michael Kubacki , Eric Dong , Ray Ni , Rahul Kumar References: <20200702051525.1102-1-guomin.jiang@intel.com> <20200702051525.1102-3-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: <15e6963c-458f-573a-d00d-bdd6f77cb4cd@redhat.com> Date: Fri, 3 Jul 2020 13:36:11 +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-3-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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 Hi, this patch contains a bunch of changes that are not related to the main purpose of the patch. See below. On 07/02/20 07:15, Guomin Jiang wrote: > From: Michael Kubacki > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614 > > Moves the GDT and IDT to permanent memory in a memory discovered > callback. This is done to ensure the GDT and IDT authenticated in > pre-memory is not fetched from outside a verified location after > the permanent memory transition. > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Signed-off-by: Michael Kubacki > --- > UefiCpuPkg/CpuMpPei/CpuMpPei.c | 40 ++++++++++++++++++- > UefiCpuPkg/CpuMpPei/CpuMpPei.h | 13 ++++++ > UefiCpuPkg/CpuMpPei/CpuPaging.c | 14 +++++-- > .../Ia32/ArchExceptionHandler.c | 4 +- > .../SecPeiCpuException.c | 2 +- > 5 files changed, 65 insertions(+), 8 deletions(-) > > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c > index 07ccbe7c6a91..2d6f1bc98851 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c > @@ -429,6 +429,44 @@ GetGdtr ( > AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer); > } > > +/** > + Migrates the Global Descriptor Table (GDT) to permanent memory. > + > + @retval EFI_SUCCESS The GDT was migrated successfully. > + @retval EFI_OUT_OF_RESOURCES The GDT could not be migrated due to lack of available memory. > + > +**/ > +EFI_STATUS > +EFIAPI > +MigrateGdt ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + UINTN GdtBufferSize; > + IA32_DESCRIPTOR Gdtr; > + UINT8 *GdtBuffer; > + > + AsmReadGdtr ((IA32_DESCRIPTOR *) &Gdtr); > + GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1; > + > + Status = PeiServicesAllocatePool ( > + GdtBufferSize, > + (VOID **) &GdtBuffer > + ); > + ASSERT (GdtBuffer != NULL); > + if (EFI_ERROR (Status)) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + GdtBuffer = ALIGN_POINTER (GdtBuffer, sizeof (IA32_TSS_DESCRIPTOR)); > + CopyMem ((VOID *) (UINTN) GdtBuffer, (VOID *) Gdtr.Base, Gdtr.Limit + 1); > + Gdtr.Base = (UINT32)(UINTN) GdtBuffer; > + AsmWriteGdtr (&Gdtr); > + > + return EFI_SUCCESS; > +} > + So this hunk relates to the main purpose of the patch; OK. > /** > Initializes CPU exceptions handlers for the sake of stack switch requirement. > > @@ -644,7 +682,7 @@ InitializeCpuMpWorker ( > &gEfiVectorHandoffInfoPpiGuid, > 0, > NULL, > - (VOID **)&VectorHandoffInfoPpi > + (VOID **) &VectorHandoffInfoPpi > ); > if (Status == EFI_SUCCESS) { > VectorInfo = VectorHandoffInfoPpi->Info; This change is both useless and totally unrelated to the patch. (1) Please drop this hunk. > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h > index 7d5c527d6006..5dc956409594 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h > @@ -397,6 +397,19 @@ SecPlatformInformation2 ( > OUT EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2 > ); > > +/** > + Migrates the Global Descriptor Table (GDT) to permanent memory. > + > + @retval EFI_SUCCESS The GDT was migrated successfully. > + @retval EFI_OUT_OF_RESOURCES The GDT could not be migrated due to lack of available memory. > + > +**/ > +EFI_STATUS > +EFIAPI > +MigrateGdt ( > + VOID > + ); > + > /** > Initializes MP and exceptions handlers. > (2) There's no need to declare this function as EFIAPI. Using EFIAPI is confusing, because it suggests that the interface is called between modules. But that's not the case, as far as I can tell. Please drop EFIAPI. > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c > index a462e7ee1e38..d0cbebf70bbf 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c > @@ -152,7 +152,7 @@ GetPhysicalAddressWidth ( > Get the type of top level page table. > > @retval Page512G PML4 paging. > - @retval Page1G PAE paing. > + @retval Page1G PAE paging. > > **/ > PAGE_ATTRIBUTE > @@ -582,7 +582,7 @@ SetupStackGuardPage ( > } > > /** > - Enabl/setup stack guard for each processor if PcdCpuStackGuard is set to TRUE. > + Enable/setup stack guard for each processor if PcdCpuStackGuard is set to TRUE. > > Doing this in the memory-discovered callback is to make sure the Stack Guard > feature to cover as most PEI code as possible. (3) These changes are valid (they are typo fixes), but they definitely belong to a separate patch. Please split them off. > @@ -602,8 +602,14 @@ MemoryDiscoveredPpiNotifyCallback ( > IN VOID *Ppi > ) > { > - EFI_STATUS Status; > - BOOLEAN InitStackGuard; > + EFI_STATUS Status; > + BOOLEAN InitStackGuard; > + BOOLEAN InterruptState; > + > + InterruptState = SaveAndDisableInterrupts (); > + Status = MigrateGdt (); > + ASSERT_EFI_ERROR (Status); > + SetInterruptState (InterruptState); > > // > // Paging must be setup first. Otherwise the exception TSS setup during MP This does belong here, OK. > diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > index 1aafb7dac139..903449e0daa9 100644 > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > @@ -18,8 +18,8 @@ > **/ > VOID > ArchUpdateIdtEntry ( > - IN IA32_IDT_GATE_DESCRIPTOR *IdtEntry, > - IN UINTN InterruptHandler > + OUT IA32_IDT_GATE_DESCRIPTOR *IdtEntry, > + IN UINTN InterruptHandler > ) > { > IdtEntry->Bits.OffsetLow = (UINT16)(UINTN)InterruptHandler; (4) Please split this off to a separate patch. (It's OK to create just one other patch named "UefiCpuPkg/CpuMpPei: trivial cleanups", and to move the IN/OUT update and the typo fixes to that patch. I'm not requesting that every trivial update be placed in its own patch, just that the trivial updates be kept separate from a patch that is supposed to fix a CVE.) > diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c > index 20148db74cf8..d4ae153c5742 100644 > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c > +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c > @@ -87,7 +87,7 @@ InitializeCpuExceptionHandlers ( > IdtEntryCount = (IdtDescriptor.Limit + 1) / sizeof (IA32_IDT_GATE_DESCRIPTOR); > if (IdtEntryCount > CPU_EXCEPTION_NUM) { > // > - // CPU exeption library only setup CPU_EXCEPTION_NUM exception handler at most > + // CPU exception library only setup CPU_EXCEPTION_NUM exception handler at most > // > IdtEntryCount = CPU_EXCEPTION_NUM; > } > (5) Same as (3). Thanks Laszlo