[edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)

Guomin Jiang posted 9 patches 4 years, 4 months ago
[edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Posted by Guomin Jiang 4 years, 4 months ago
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
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;
   }
-- 
2.25.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61943): https://edk2.groups.io/g/devel/message/61943
Mute This Topic: https://groups.io/mt/75252661/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Posted by Laszlo Ersek 4 years, 4 months ago
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 <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;
> +}
> +

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62025): https://edk2.groups.io/g/devel/message/62025
Mute This Topic: https://groups.io/mt/75252661/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Posted by Laszlo Ersek 4 years, 4 months ago
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 <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;

(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 **) &GdtBuffer
>> +              );

(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 (&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
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62027): https://edk2.groups.io/g/devel/message/62027
Mute This Topic: https://groups.io/mt/75252661/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Posted by Laszlo Ersek 4 years, 4 months ago
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?

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;
>    }
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62033): https://edk2.groups.io/g/devel/message/62033
Mute This Topic: https://groups.io/mt/75252661/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Posted by Laszlo Ersek 4 years, 4 months ago
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;
>>    }
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62040): https://edk2.groups.io/g/devel/message/62040
Mute This Topic: https://groups.io/mt/75252661/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
Posted by Ni, Ray 4 years, 4 months ago
Guomin,
Can you please separate the coding style change and the functionality change in different patches?


> -----Original Message-----
> From: devel@edk2.groups.io <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 <michael.a.kubacki@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)
> 
> 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
> 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;
>    }
> --
> 2.25.1.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61965): https://edk2.groups.io/g/devel/message/61965
Mute This Topic: https://groups.io/mt/75252661/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-