public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>,
	"Laszlo Ersek (lersek@redhat.com)" <lersek@redhat.com>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Fan, Jeff" <jeff.fan@intel.com>
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault.
Date: Thu, 24 Nov 2016 00:45:49 +0000	[thread overview]
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C50386D9CED@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com>

Thank you, Laszlo.

I will check in debug message enhancement at first, with your suggestion to remove the last superfluous argument and use small 'x' consistently. :)


Your regression test is very nice.
For this patch, I will invite more people to review and validation to see if there is other concern.



Thank you
Yao Jiewen


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, November 24, 2016 6:29 AM
To: Yao, Jiewen; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>
Cc: Kinney, Michael D; Fan, Jeff
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault.

On 11/23/16 15:54, Jiewen Yao wrote:
> This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=246
>
> Previously, when SMM exception happens after EndOfDxe,
> with StackGuard enabled on IA32, the #double fault exception
> is reported instead of #page fault.
>
> Root cause is below:
>
> Current EDKII SMM page protection will lock GDT.
> If IA32 stack guard is enabled, the page fault handler will do task switch.
> This task switch need write busy flag in GDT, and write TSS.
>
> However, the GDT and TSS is locked at that time, so the
> double fault happens.
>
> This issue does not exist on X64, or IA32 without StackGuard.
>
> In this enhanced solution, we create a special GDT whose CS/DS is
> at read-only page, and whose TSS segment is at read-write page.
> We also put TSS into a read-write page.
>
> The detail is at UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c.
>
> Then the normal code can being protected, because it does not touch TSS.
> When page fault happens, the TSS region can be written.
>
> This fix is IA32 StackGuard specific.
>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>
> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S          |   2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm        |   2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm       |   2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S      |   4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm    |   4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm   |   4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c      | 207 ++++++++++++++++----
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |   7 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  78 +++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |  48 -----
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c       |  67 ++++++-
>  11 files changed, 311 insertions(+), 114 deletions(-)

I'm not volunteering for a review (I have no clue what a task switch
is). I've done regression testing with Ia32 and Ia32X64 OVMF, including
repeated suspend/resume.

Regression-tested-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>

In addition, let me point out that my recent testing of the patch that
improves the SMM exception messages:

[edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message
[edk2] [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message.

covered the SMM stack overflow case, but -- as I stated explicitly --
only in the Ia32X64 build. See bullet (2) in
<https://lists.01.org/pipermail/edk2-devel/2016-November/005078.html>.

The reason is that the SMM stack overflow has occurred to us only when
running the EnrollDefaultKeys.efi application, and for that app we only
have a "production use" under Ia32X64, not Ia32.

Anyway, I have now applied your

  [edk2] [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message.

together with this patch, and attempted to trigger SMM stack overflow on
both Ia32 and Ia32X64.

- I confirm that the Ia32X64 case continues to work, like before:
<https://lists.01.org/pipermail/edk2-devel/2016-November/005133.html>.
For that,

Regression-tested-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>

- Regarding the Ia32 case, which I haven't tested before, I couldn't
trigger an SMM stack overflow with EnrollDefaultKeys.efi, even after
reverting the patch that increased the SMM stack size in OVMF
(0d0c245dfb14). So, I think I cannot give my Tested-by (I couldn't test
the actual new functionality), just my Regression-tested-by (see above).

Thanks
Laszlo

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> index 0c07558..e27080d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> @@ -41,7 +41,7 @@ ASM_GLOBAL  ASM_PFX(gSmiHandlerIdtr)
>
>  .equ            PROTECT_MODE_CS, 0x08
>  .equ            PROTECT_MODE_DS, 0x20
> -.equ            TSS_SEGMENT,     0x40
> +.equ            TSS_SEGMENT,     0x1000
>
>      .text
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> index eda1708..e1ab470 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> @@ -36,7 +36,7 @@ DSC_OTHERSEG  EQU     20
>
>  PROTECT_MODE_CS EQU   08h
>  PROTECT_MODE_DS EQU   20h
> -TSS_SEGMENT     EQU   40h
> +TSS_SEGMENT     EQU   1000h
>
>  SmiRendezvous      PROTO   C
>  CpuSmmDebugEntry   PROTO   C
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> index d50a317..ccfb0d4 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> @@ -32,7 +32,7 @@
>
>  %define PROTECT_MODE_CS 0x8
>  %define PROTECT_MODE_DS 0x20
> -%define TSS_SEGMENT 0x40
> +%define TSS_SEGMENT 0x1000
>
>  extern ASM_PFX(SmiRendezvous)
>  extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard))
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S
> index cf5ef82..30dd25e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S
> @@ -99,8 +99,8 @@ ExceptionTssSeg:
>
>  .equ  CODE_SEL,          CodeSeg32 - NullSeg
>  .equ  DATA_SEL,          DataSeg32 - NullSeg
> -.equ  TSS_SEL,           TssSeg - NullSeg
> -.equ  EXCEPTION_TSS_SEL, ExceptionTssSeg - NullSeg
> +.equ  TSS_SEL,           0x1000
> +.equ  EXCEPTION_TSS_SEL, 0x1008
>
>  # IA32 TSS fields
>  .equ  TSS_ESP0,          4
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm
> index 7b162f8..740e4a5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm
> @@ -102,8 +102,8 @@ ExceptionTssSeg     LABEL   QWORD
>
>  CODE_SEL          = offset CodeSeg32 - offset NullSeg
>  DATA_SEL          = offset DataSeg32 - offset NullSeg
> -TSS_SEL           = offset TssSeg - offset NullSeg
> -EXCEPTION_TSS_SEL = offset ExceptionTssSeg - offset NullSeg
> +TSS_SEL               EQU   1000h
> +EXCEPTION_TSS_SEL     EQU   1008h
>
>  IA32_TSS STRUC
>                      DW ?
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm
> index 4d58999..a6dcf47 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm
> @@ -98,8 +98,8 @@ ExceptionTssSeg:
>
>  CODE_SEL          equ CodeSeg32 - NullSeg
>  DATA_SEL          equ DataSeg32 - NullSeg
> -TSS_SEL           equ TssSeg - NullSeg
> -EXCEPTION_TSS_SEL equ ExceptionTssSeg - NullSeg
> +%define TSS_SEL           0x1000
> +%define EXCEPTION_TSS_SEL 0x1008
>
>  struc IA32_TSS
>                      resw 1
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> index f4db6c8..8e25b49 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> @@ -43,17 +43,15 @@ InitializeIDTSmmStackGuard (
>
>  /**
>    Initialize Gdt for all processors.
> -
> +
>    @param[in]   Cr3          CR3 value.
> -  @param[out]  GdtStepSize  The step size for GDT table.
>
> -  @return GdtBase for processor 0.
> -          GdtBase for processor X is: GdtBase + (GdtStepSize * X)
> +  @return A GdtArray to hold all base of Gdt Table for all processors.
> +          GdtBase for processor X is: GdtArray[X]
>  **/
> -VOID *
> +UINTN *
>  InitGdt (
> -  IN  UINTN  Cr3,
> -  OUT UINTN  *GdtStepSize
> +  IN  UINTN  Cr3
>    )
>  {
>    UINTN                     Index;
> @@ -62,6 +60,16 @@ InitGdt (
>    UINTN                     GdtTssTableSize;
>    UINT8                     *GdtTssTables;
>    UINTN                     GdtTableStepSize;
> +  UINTN                     *GdtArray;
> +  UINTN                     GdtCountPerPage;
> +  UINTN                     BundleCount;
> +  UINTN                     BundleIndex;
> +  UINT8                     *BundleBase;
> +  UINTN                     GdtIndex;
> +  UINTN                     CountMax;
> +
> +  GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
> +  ASSERT (GdtArray != NULL);
>
>    if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
>      //
> @@ -71,41 +79,92 @@ InitGdt (
>      //
>
>      //
> -    // Enlarge GDT to contain 2 TSS descriptors
> +    // We must separate normal GDT entry from GDT_TSS entry.
> +    // The formal could be read only, and the later must be readwrite.
> +    // The final layout is below:
> +    //
> +    // |<------ Read Only Bundle ------>|<--------- Read Write Bundle -------------->|
> +    // |                                |                                            |
> +    // +--------------------------------+============================================+--------------+==================+
> +    // | GDT(0) | GDT(1) | ... | GDT(n) | GDT_TSS(0) | GDT_TSS(1) | ... | GDT_TSS(n) | GDT(n+1) |...| GDT_TSS(n+1) |...|
> +    // +--------------------------------+============================================+--------------+==================+
> +    // |                                     |       |
> +    // |<------------ Gdt.Limit ------------>|<-TSS->|
>      //
> -    gcSmiGdtr.Limit += (UINT16)(2 * sizeof (IA32_SEGMENT_DESCRIPTOR));
>
> -    GdtTssTableSize = (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned
> -    mGdtBufferSize = GdtTssTableSize * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus;
> -    GdtTssTables = (UINT8*)AllocateCodePages (EFI_SIZE_TO_PAGES (mGdtBufferSize));
> +    GdtTableStepSize = (2 * sizeof (IA32_SEGMENT_DESCRIPTOR) + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned
> +    if (GdtTableStepSize < (UINTN)(gcSmiGdtr.Limit + 1)) {
> +      GdtTableStepSize = gcSmiGdtr.Limit + 1;
> +    }
> +    GdtCountPerPage = EFI_PAGE_SIZE / GdtTableStepSize;
> +    BundleCount = (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus + GdtCountPerPage - 1) / GdtCountPerPage;
> +    mGdtBufferSize = EFI_PAGES_TO_SIZE(BundleCount * 2);
> +
> +    GdtTssTables = (UINT8*)AllocateCodePages (BundleCount * 2);
>      ASSERT (GdtTssTables != NULL);
> +    ZeroMem ((VOID *)GdtTssTables, EFI_PAGES_TO_SIZE(BundleCount * 2));
>      mGdtBuffer = (UINTN)GdtTssTables;
> -    GdtTableStepSize = GdtTssTableSize;
>
> -    for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) {
> -      CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1 + TSS_SIZE * 2);
> -      //
> -      // Fixup TSS descriptors
> -      //
> -      TssBase = (UINTN)(GdtTssTables + GdtTableStepSize * Index + gcSmiGdtr.Limit + 1);
> -      GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2;
> -      GdtDescriptor->Bits.BaseLow = (UINT16)TssBase;
> -      GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16);
> -      GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24);
> -
> -      TssBase += TSS_SIZE;
> -      GdtDescriptor++;
> -      GdtDescriptor->Bits.BaseLow = (UINT16)TssBase;
> -      GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16);
> -      GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24);
> -      //
> -      // Fixup TSS segments
> -      //
> -      // ESP as known good stack
> -      //
> -      *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) =  mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize;
> -      *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3;
> +    Index = 0;
> +    for (BundleIndex = 0; BundleIndex < BundleCount; BundleIndex++) {
> +      CountMax = GdtCountPerPage;
> +      if (BundleIndex == BundleCount - 1) {
> +        CountMax = gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus - GdtCountPerPage * BundleIndex;
> +      }
> +      BundleBase = GdtTssTables + EFI_PAGE_SIZE * 2 * BundleIndex;
> +      for (GdtIndex = 0; GdtIndex < CountMax; GdtIndex++, Index++) {
> +        GdtArray[Index] = (UINTN)BundleBase + GdtTableStepSize * GdtIndex;
> +        //
> +        // Copy Basic GDT
> +        //
> +        CopyMem (
> +          (VOID *)GdtArray[Index],
> +          (VOID*)(UINTN)gcSmiGdtr.Base,
> +          gcSmiGdtr.Limit + 1
> +          );
> +        //
> +        // Copy GDT TSS segment
> +        //
> +        CopyMem (
> +          (VOID *)(GdtArray[Index] + EFI_PAGE_SIZE),
> +          (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1),
> +          sizeof(IA32_SEGMENT_DESCRIPTOR) * 2
> +          );
> +        //
> +        // Copy TSS
> +        //
> +        TssBase = (UINTN)(GdtArray[Index] + EFI_PAGE_SIZE + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2);
> +        CopyMem (
> +          (VOID *)(TssBase),
> +          (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1 + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2),
> +          TSS_SIZE * 2
> +          );
> +        //
> +        // Fixup TSS descriptors
> +        //
> +        GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2;
> +        GdtDescriptor->Bits.BaseLow = (UINT16)TssBase;
> +        GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16);
> +        GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24);
> +
> +        TssBase += TSS_SIZE;
> +        GdtDescriptor++;
> +        GdtDescriptor->Bits.BaseLow = (UINT16)TssBase;
> +        GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16);
> +        GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24);
> +        //
> +        // Fixup TSS segments
> +        //
> +        // ESP as known good stack
> +        //
> +        *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) =  mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize;
> +        *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3;
> +      }
>      }
> +    //
> +    // Enlarge GDT to contain 1 page plus 2 TSS descriptors
> +    //
> +    gcSmiGdtr.Limit = (UINT16)(EFI_PAGE_SIZE + 2 * sizeof (IA32_SEGMENT_DESCRIPTOR) - 1);
>    } else {
>      //
>      // Just use original table, AllocatePage and copy them here to make sure GDTs are covered in page memory.
> @@ -119,11 +178,83 @@ InitGdt (
>
>      for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) {
>        CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1);
> +      GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index;
> +    }
> +  }
> +
> +  return GdtArray;
> +}
> +
> +/**
> +  This function sets GDT/IDT buffer to be RO and XP.
> +**/
> +VOID
> +PatchGdtIdtMap (
> +  VOID
> +  )
> +{
> +  EFI_PHYSICAL_ADDRESS       BaseAddress;
> +  UINTN                      Size;
> +  UINTN                      Index;
> +  UINTN                      BundleCount;
> +
> +  //
> +  // GDT
> +  //
> +  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));
> +
> +  if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
> +    BaseAddress = mGdtBuffer;
> +    Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);
> +    SmmSetMemoryAttributes (
> +      BaseAddress,
> +      Size,
> +      EFI_MEMORY_XP
> +      );
> +    BundleCount = EFI_SIZE_TO_PAGES(mGdtBufferSize)/2;
> +    for (Index = 0; Index < BundleCount; Index++) {
> +      SmmSetMemoryAttributes (
> +        BaseAddress + EFI_PAGE_SIZE * Index * 2,
> +        EFI_PAGE_SIZE,
> +        EFI_MEMORY_RO
> +        );
> +      SmmClearMemoryAttributes (
> +        BaseAddress + EFI_PAGE_SIZE * (Index * 2 + 1),
> +        EFI_PAGE_SIZE,
> +        EFI_MEMORY_RO
> +        );
>      }
> +  } else {
> +    BaseAddress = mGdtBuffer;
> +    Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);
> +    SmmSetMemoryAttributes (
> +      BaseAddress,
> +      Size,
> +      EFI_MEMORY_RO
> +      );
> +    SmmSetMemoryAttributes (
> +      BaseAddress,
> +      Size,
> +      EFI_MEMORY_XP
> +      );
>    }
> +  //
> +  // IDT
> +  //
> +  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));
>
> -  *GdtStepSize = GdtTableStepSize;
> -  return GdtTssTables;
> +  BaseAddress = gcSmiIdtr.Base;
> +  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);
> +  SmmSetMemoryAttributes (
> +    BaseAddress,
> +    Size,
> +    EFI_MEMORY_RO
> +    );
> +  SmmSetMemoryAttributes (
> +    BaseAddress,
> +    Size,
> +    EFI_MEMORY_XP
> +    );
>  }
>
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 9b8db90..591a18c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1377,8 +1377,7 @@ InitializeMpServiceData (
>    UINTN                     Index;
>    MTRR_SETTINGS             *Mtrr;
>    PROCESSOR_SMM_DESCRIPTOR  *Psd;
> -  UINT8                     *GdtTssTables;
> -  UINTN                     GdtTableStepSize;
> +  UINTN                     *GdtArray;
>
>    //
>    // Allocate memory for all locks and semaphores
> @@ -1407,7 +1406,7 @@ InitializeMpServiceData (
>    //
>    Cr3 = SmmInitPageTable ();
>
> -  GdtTssTables = InitGdt (Cr3, &GdtTableStepSize);
> +  GdtArray = InitGdt (Cr3);
>
>    //
>    // Initialize PROCESSOR_SMM_DESCRIPTOR for each CPU
> @@ -1415,7 +1414,7 @@ InitializeMpServiceData (
>    for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
>      Psd = (PROCESSOR_SMM_DESCRIPTOR *)(VOID *)(UINTN)(mCpuHotPlugData.SmBase[Index] + SMM_PSD_OFFSET);
>      CopyMem (Psd, &gcPsd, sizeof (gcPsd));
> -    Psd->SmmGdtPtr = (UINT64)(UINTN)(GdtTssTables + GdtTableStepSize * Index);
> +    Psd->SmmGdtPtr = (UINT64)(UINTN)GdtArray[Index];
>      Psd->SmmGdtSize = gcSmiGdtr.Limit + 1;
>
>      //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index abe5cc6..9e00022 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -512,15 +512,21 @@ InitializeIDTSmmStackGuard (
>    Initialize Gdt for all processors.
>
>    @param[in]   Cr3          CR3 value.
> -  @param[out]  GdtStepSize  The step size for GDT table.
>
> -  @return GdtBase for processor 0.
> -          GdtBase for processor X is: GdtBase + (GdtStepSize * X)
> +  @return A GdtArray to hold all base of Gdt Table for all processors.
> +          GdtBase for processor X is: GdtArray[X]
>  **/
> -VOID *
> +UINTN *
>  InitGdt (
> -  IN  UINTN  Cr3,
> -  OUT UINTN  *GdtStepSize
> +  IN  UINTN  Cr3
> +  );
> +
> +/**
> +  This function sets GDT/IDT buffer to be RO and XP.
> +**/
> +VOID
> +PatchGdtIdtMap (
> +  VOID
>    );
>
>  /**
> @@ -596,6 +602,66 @@ SmmBlockingStartupThisAp (
>    );
>
>  /**
> +  This function sets the attributes for the memory region specified by BaseAddress and
> +  Length from their current attributes to the attributes specified by Attributes.
> +
> +  @param[in]  BaseAddress      The physical address that is the start address of a memory region.
> +  @param[in]  Length           The size in bytes of the memory region.
> +  @param[in]  Attributes       The bit mask of attributes to set for the memory region.
> +
> +  @retval EFI_SUCCESS           The attributes were set for the memory region.
> +  @retval EFI_ACCESS_DENIED     The attributes for the memory resource range specified by
> +                                BaseAddress and Length cannot be modified.
> +  @retval EFI_INVALID_PARAMETER Length is zero.
> +                                Attributes specified an illegal combination of attributes that
> +                                cannot be set together.
> +  @retval EFI_OUT_OF_RESOURCES  There are not enough system resources to modify the attributes of
> +                                the memory resource range.
> +  @retval EFI_UNSUPPORTED       The processor does not support one or more bytes of the memory
> +                                resource range specified by BaseAddress and Length.
> +                                The bit mask of attributes is not support for the memory resource
> +                                range specified by BaseAddress and Length.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SmmSetMemoryAttributes (
> +  IN  EFI_PHYSICAL_ADDRESS                       BaseAddress,
> +  IN  UINT64                                     Length,
> +  IN  UINT64                                     Attributes
> +  );
> +
> +/**
> +  This function clears the attributes for the memory region specified by BaseAddress and
> +  Length from their current attributes to the attributes specified by Attributes.
> +
> +  @param[in]  BaseAddress      The physical address that is the start address of a memory region.
> +  @param[in]  Length           The size in bytes of the memory region.
> +  @param[in]  Attributes       The bit mask of attributes to clear for the memory region.
> +
> +  @retval EFI_SUCCESS           The attributes were cleared for the memory region.
> +  @retval EFI_ACCESS_DENIED     The attributes for the memory resource range specified by
> +                                BaseAddress and Length cannot be modified.
> +  @retval EFI_INVALID_PARAMETER Length is zero.
> +                                Attributes specified an illegal combination of attributes that
> +                                cannot be set together.
> +  @retval EFI_OUT_OF_RESOURCES  There are not enough system resources to modify the attributes of
> +                                the memory resource range.
> +  @retval EFI_UNSUPPORTED       The processor does not support one or more bytes of the memory
> +                                resource range specified by BaseAddress and Length.
> +                                The bit mask of attributes is not support for the memory resource
> +                                range specified by BaseAddress and Length.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SmmClearMemoryAttributes (
> +  IN  EFI_PHYSICAL_ADDRESS                       BaseAddress,
> +  IN  UINT64                                     Length,
> +  IN  UINT64                                     Attributes
> +  );
> +
> +/**
>    Initialize MP synchronization data.
>
>  **/
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index c85e025..fc440e4 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -750,54 +750,6 @@ PatchSmmSaveStateMap (
>  }
>
>  /**
> -  This function sets GDT/IDT buffer to be RO and XP.
> -**/
> -VOID
> -PatchGdtIdtMap (
> -  VOID
> -  )
> -{
> -  EFI_PHYSICAL_ADDRESS       BaseAddress;
> -  UINTN                      Size;
> -
> -  //
> -  // GDT
> -  //
> -  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));
> -
> -  BaseAddress = mGdtBuffer;
> -  Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);
> -  SmmSetMemoryAttributes (
> -    BaseAddress,
> -    Size,
> -    EFI_MEMORY_RO
> -    );
> -  SmmSetMemoryAttributes (
> -    BaseAddress,
> -    Size,
> -    EFI_MEMORY_XP
> -    );
> -
> -  //
> -  // IDT
> -  //
> -  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));
> -
> -  BaseAddress = gcSmiIdtr.Base;
> -  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);
> -  SmmSetMemoryAttributes (
> -    BaseAddress,
> -    Size,
> -    EFI_MEMORY_RO
> -    );
> -  SmmSetMemoryAttributes (
> -    BaseAddress,
> -    Size,
> -    EFI_MEMORY_XP
> -    );
> -}
> -
> -/**
>    This function sets memory attribute according to MemoryAttributesTable.
>  **/
>  VOID
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> index 9fc00c1..baeec4a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> @@ -42,15 +42,13 @@ InitializeIDTSmmStackGuard (
>    Initialize Gdt for all processors.
>
>    @param[in]   Cr3          CR3 value.
> -  @param[out]  GdtStepSize  The step size for GDT table.
>
> -  @return GdtBase for processor 0.
> -          GdtBase for processor X is: GdtBase + (GdtStepSize * X)
> +  @return A GdtArray to hold all base of Gdt Table for all processors.
> +          GdtBase for processor X is: GdtArray[X]
>  **/
> -VOID *
> +UINTN *
>  InitGdt (
> -  IN  UINTN  Cr3,
> -  OUT UINTN  *GdtStepSize
> +  IN  UINTN  Cr3
>    )
>  {
>    UINTN                     Index;
> @@ -59,6 +57,10 @@ InitGdt (
>    UINTN                     GdtTssTableSize;
>    UINT8                     *GdtTssTables;
>    UINTN                     GdtTableStepSize;
> +  UINTN                     *GdtArray;
> +
> +  GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
> +  ASSERT (GdtArray != NULL);
>
>    //
>    // For X64 SMM, we allocate separate GDT/TSS for each CPUs to avoid TSS load contention
> @@ -89,10 +91,58 @@ InitGdt (
>        //
>        *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) = (mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize);
>      }
> +    GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index;
>    }
>
> -  *GdtStepSize = GdtTableStepSize;
> -  return GdtTssTables;
> +  return GdtArray;
> +}
> +
> +/**
> +  This function sets GDT/IDT buffer to be RO and XP.
> +**/
> +VOID
> +PatchGdtIdtMap (
> +  VOID
> +  )
> +{
> +  EFI_PHYSICAL_ADDRESS       BaseAddress;
> +  UINTN                      Size;
> +
> +  //
> +  // GDT
> +  //
> +  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));
> +
> +  BaseAddress = mGdtBuffer;
> +  Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);
> +  SmmSetMemoryAttributes (
> +    BaseAddress,
> +    Size,
> +    EFI_MEMORY_RO
> +    );
> +  SmmSetMemoryAttributes (
> +    BaseAddress,
> +    Size,
> +    EFI_MEMORY_XP
> +    );
> +
> +  //
> +  // IDT
> +  //
> +  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));
> +
> +  BaseAddress = gcSmiIdtr.Base;
> +  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);
> +  SmmSetMemoryAttributes (
> +    BaseAddress,
> +    Size,
> +    EFI_MEMORY_RO
> +    );
> +  SmmSetMemoryAttributes (
> +    BaseAddress,
> +    Size,
> +    EFI_MEMORY_XP
> +    );
>  }
>
>  /**
> @@ -154,4 +204,3 @@ TransferApToSafeState (
>    ASSERT (FALSE);
>  }
>
> -
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel


      parent reply	other threads:[~2016-11-24  0:46 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-23 14:54 [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault Jiewen Yao
2016-11-23 22:28 ` Laszlo Ersek
     [not found]   ` <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com>
2016-11-24  0:45     ` Yao, Jiewen [this message]

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=74D8A39837DF1E4DA445A8C0B3885C50386D9CED@shsmsx102.ccr.corp.intel.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