Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
On 07/03/20 15:57, Laszlo Ersek wrote:
> 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 *) );
>> + GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
>> +
>> + Status = PeiServicesAllocatePool (
>> + GdtBufferSize,
>> + (VOID **)
>> + );
>> + 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 ();
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>> /**
>>Initializes CPU exceptions handlers for the sake of stack switch
>> requirement.
>>
>> @@ -644,7 +682,7 @@ InitializeCpuMpWorker (
>> ,
>> 0,
>> NULL,
>> - (VOID **)
>> + (VOID **)
>> );
>>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 Page1GPAE paing.
>> + @retval Page1GPAE 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
Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
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 *) );
> + GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
> +
> + Status = PeiServicesAllocatePool (
> + GdtBufferSize,
> + (VOID **)
> + );
> + 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 ();
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
>Initializes CPU exceptions handlers for the sake of stack switch
> requirement.
>
> @@ -644,7 +682,7 @@ InitializeCpuMpWorker (
> ,
> 0,
> NULL,
> - (VOID **)
> + (VOID **)
> );
>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 Page1GPAE paing.
> + @retval Page1GPAE 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?
Thanks
Laszlo
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> index 1aafb7dac139..903449e0daa9 100644
> ---
Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Hi,
more comments on the MigrateGdt() function:
On 07/03/20 13:36, Laszlo Ersek wrote:
> 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 *) );
>> + GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
(6) I don't understand why IA32_TSS_DESCRIPTOR is used in this function.
While technically it should not cause problems, "TSS" seems completely
irrelevant here. Please use IA32_SEGMENT_DESCRIPTOR instead.
(7) The buffer size is too large. "Gdtr.Limit" is an inclusive limit
indeed, so the "+1" at the end is justified, for determining the size.
However, for the alignment, we only need *at most*
sizeof (IA32_SEGMENT_DESCRIPTOR) - 1
bytes. (If the allocation is immediately well-aligned, then we need 0
bytes for ensuring the proper alignment, and not "sizeof
(IA32_SEGMENT_DESCRIPTOR)" bytes.)
So I suggest
GdtBufferSize = sizeof (IA32_SEGMENT_DESCRIPTOR) - 1 + Gdtr.Limit + 1;
>> +
>> + Status = PeiServicesAllocatePool (
>> + GdtBufferSize,
>> + (VOID **)
>> + );
(8) We have way too much casting back and forth here. Just make
"GdtBuffer" a (VOID*), and then you can drop this cast.
>> + ASSERT (GdtBuffer != NULL);
>> + if (EFI_ERROR (Status)) {
>> +return EFI_OUT_OF_RESOURCES;
>> + }
>> +
>> + GdtBuffer = ALIGN_POINTER (GdtBuffer, sizeof (IA32_TSS_DESCRIPTOR));
(9) Same as (6).
>> + CopyMem ((VOID *) (UINTN) GdtBuffer, (VOID *) Gdtr.Base, Gdtr.Limit + 1);
(10) Same as (8); drop the (VOID *)(UINTN) casts, please.
>> + Gdtr.Base = (UINT32)(UINTN) GdtBuffer;
(11) The (UINT32) cast is wrong, please drop it. "Gdtr.Base" has type UINTN.
Thanks
Laszlo
>> + AsmWriteGdtr ();
>> +
>> + 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 (
>> ,
>> 0,
>> NULL,
>> - (VOID **)
>> + (VOID **)
>> );
>>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
Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
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 *) );
> + GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
> +
> + Status = PeiServicesAllocatePool (
> + GdtBufferSize,
> + (VOID **)
> + );
> + 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 ();
> +
> + 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 (
> ,
> 0,
> NULL,
> - (VOID **)
> + (VOID **)
> );
>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 Page1GPAE paing.
> + @retval Page1GPAE 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 =
Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Guomin,
Can you please separate the coding style change and the functionality change in
different patches?
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Guomin Jiang
> Sent: Thursday, July 2, 2020 1:15 PM
> To: devel@edk2.groups.io
> Cc: Michael Kubacki ; Dong, Eric
> ; Ni, Ray ; Laszlo
> Ersek ; Kumar, Rahul1
> Subject: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT
> migration support (CVE-2019-11098)
>
> 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 *) );
> + GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
> +
> + Status = PeiServicesAllocatePool (
> + GdtBufferSize,
> + (VOID **)
> + );
> + 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 ();
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
>Initializes CPU exceptions handlers for the sake of stack switch
> requirement.
>
> @@ -644,7 +682,7 @@ InitializeCpuMpWorker (
> ,
> 0,
> NULL,
> - (VOID **)
> + (VOID **)
> );
>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 Page1GPAE paing.
> + @retval Page1GPAE 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 @@ MemoryDiscoveredPpiNotifyC
[edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
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 *) );
+ GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
+
+ Status = PeiServicesAllocatePool (
+ GdtBufferSize,
+ (VOID **)
+ );
+ 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 ();
+
+ return EFI_SUCCESS;
+}
+
/**
Initializes CPU exceptions handlers for the sake of stack switch requirement.
@@ -644,7 +682,7 @@ InitializeCpuMpWorker (
,
0,
NULL,
- (VOID **)
+ (VOID **)
);
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 Page1GPAE paing.
+ @retval Page1GPAE 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
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
