From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, guomin.jiang@intel.com
Cc: Michael Kubacki <michael.a.kubacki@intel.com>,
Eric Dong <eric.dong@intel.com>, Ray Ni <ray.ni@intel.com>,
Rahul Kumar <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Date: Fri, 3 Jul 2020 16:33:55 +0200 [thread overview]
Message-ID: <6afb9b52-d5f7-fe71-be74-0ea51995bcb7@redhat.com> (raw)
In-Reply-To: <e60f0b07-dea9-47b7-5485-c1659eb4c19a@redhat.com>
On 07/03/20 15:57, Laszlo Ersek wrote:
> On 07/02/20 07:15, Guomin Jiang wrote:
>> From: Michael Kubacki <michael.a.kubacki@intel.com>
>>
>> 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 <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
>> ---
>> 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;
>> +}
>> +
>> /**
>> 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;
>> 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.
>>
>> 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.
>> @@ -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
>
> (12) The GDT migration should be made dependent on
> "PcdMigrateTemporaryRamFirmwareVolumes", shouldn't it?
... Or is this *another* preexistent bug that we should fix regardless
of the "temporary RAM evacuation" feature?
I mean, considering current master, once we switch to permanent PEI RAM,
do we still rely on a GDT that lives in temp RAM?
If that's the case, then we should even split this series into two
series. The first series should fix the other issues first -- typos,
IN/OUT mistakes, this GDT problem, and the S3 shadowing bug in the DXE
IPL PEIM.
Once all that is done, we can introduce
"PcdMigrateTemporaryRamFirmwareVolumes", and the dependent new feature.
Thanks
Laszlo
>> 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;
>> 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;
>> }
>>
>
next prev parent reply other threads:[~2020-07-03 14:34 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 5:15 [PATCH v2 0/9] Migrate Pointer from flash to permanent memory (CVE-2019-11098) Guomin Jiang
2020-07-02 5:15 ` [PATCH v2 1/9] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore (CVE-2019-11098) Guomin Jiang
2020-07-03 12:22 ` [edk2-devel] " Laszlo Ersek
2020-07-03 13:52 ` Laszlo Ersek
2020-07-02 5:15 ` [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098) Guomin Jiang
2020-07-02 7:36 ` [edk2-devel] " Ni, Ray
2020-07-03 11:36 ` Laszlo Ersek
2020-07-03 11:52 ` Laszlo Ersek
2020-07-03 13:57 ` Laszlo Ersek
2020-07-03 14:33 ` Laszlo Ersek [this message]
2020-07-02 5:15 ` [PATCH v2 3/9] UefiCpuPkg/SecMigrationPei: Add initial PEIM (CVE-2019-11098) Guomin Jiang
2020-07-03 11:38 ` [edk2-devel] " Laszlo Ersek
2020-07-02 5:15 ` [PATCH v2 4/9] MdeModulePkg/DxeIplPeim: Register for shadow on S3 shadowed boot (CVE-2019-11098) Guomin Jiang
2020-07-03 14:00 ` [edk2-devel] " Laszlo Ersek
2020-07-03 14:23 ` Laszlo Ersek
2020-07-02 5:15 ` [PATCH v2 5/9] MdeModulePkg/Core: Create Migrated FV Info Hob for calculating hash (CVE-2019-11098) Guomin Jiang
2020-07-03 14:03 ` [edk2-devel] " Laszlo Ersek
2020-07-02 5:15 ` [PATCH v2 6/9] SecurityPkg/Tcg2Pei: Use " Guomin Jiang
2020-07-02 5:15 ` [PATCH v2 7/9] MdeModulePkg/Core: Add switch to enable or disable TOCTOU feature (CVE-2019-11098) Guomin Jiang
2020-07-03 12:48 ` [edk2-devel] " Laszlo Ersek
2020-07-02 5:15 ` [PATCH v2 8/9] UefiCpuPkg/SecMigrationPei: Add switch to control if produce PPI (CVE-2019-11098) Guomin Jiang
2020-07-03 14:05 ` [edk2-devel] " Laszlo Ersek
2020-07-02 5:15 ` [PATCH v2 9/9] UefiCpuPkg/CpuMpPei: Enable paging and set NP flag to avoid TOCTOU (CVE-2019-11098) Guomin Jiang
2020-07-03 13:11 ` [edk2-devel] " Laszlo Ersek
2020-07-03 14:06 ` [edk2-devel] [PATCH v2 0/9] Migrate Pointer from flash to permanent memory (CVE-2019-11098) Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6afb9b52-d5f7-fe71-be74-0ea51995bcb7@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox