From: "Zeng, Star" <star.zeng@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
"Dong, Eric" <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32
Date: Fri, 12 Jan 2018 08:00:08 +0000 [thread overview]
Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9FBB23@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AA7B054@shsmsx102.ccr.corp.intel.com>
Yes, right. Patch for that case is on the way.
Thanks,
Star
-----Original Message-----
From: Yao, Jiewen
Sent: Friday, January 12, 2018 3:59 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Wang, Jian J <jian.j.wang@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32
Thanks to fix this.
The update seems good. Reviewed-by: Jiewen.yao@intel.com
I reviewed more code and find we already set IDT to be RO.
gcSmiIdtr.Base = (UINTN)AllocateCodePages (EFI_SIZE_TO_PAGES(gcSmiIdtr.Limit + 1));
So we do not need set RO for IDT as well. (Setting XP is still needed).
> + BaseAddress = gcSmiIdtr.Base;
> + Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);
> + SmmSetMemoryAttributes (
> + BaseAddress,
> + Size,
> + EFI_MEMORY_RO
> + );
Thank you
Yao Jiewen
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, January 12, 2018 3:30 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on
> #page fault for IA32
>
> When StackGuard is enabled on IA32, the #double fault exception is
> reported instead of #page fault.
>
> This issue does not exist on X64, or IA32 without StackGuard.
>
> The fix at e4435f710cea2d2f10cd7343d545920867780086 was incomplete.
>
> It is because AllocateCodePages() is used to allocate buffer for GDT
> and TSS, the code pages will be set to RO in SetMemMapAttributes().
> But IA32 Stack Guard need use task switch to switch stack that need
> write GDT and TSS, so AllocateCodePages() could not be used.
>
> This patch uses AllocatePages() instead of AllocateCodePages() to
> allocate buffer for GDT and TSS if StackGuard is enabled on IA32.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 64
> +++-------------------
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 49
> ++++++++++++++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 50
> +----------------
> 4 files changed, 57 insertions(+), 116 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> index 3c68c970245f..4c1499939b1b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> @@ -1,7 +1,7 @@
> /** @file
> SMM CPU misc functions for Ia32 arch specific.
>
> -Copyright (c) 2015 - 2016, Intel Corporation. All rights
> reserved.<BR>
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights
> +reserved.<BR>
> This program and the accompanying materials are licensed and made
> available under the terms and conditions of the BSD License which
> accompanies this distribution. The full text of the license may be
> found at @@ -77,7 +77,12 @@ InitGdt (
>
> 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));
> + //
> + // IA32 Stack Guard need use task switch to switch stack that need
> + // write GDT and TSS, so AllocateCodePages() could not be used here
> + // as code pages will be set to RO.
> + //
> + GdtTssTables = (UINT8*)AllocatePages (EFI_SIZE_TO_PAGES
> (mGdtBufferSize));
> ASSERT (GdtTssTables != NULL);
> mGdtBuffer = (UINTN)GdtTssTables;
> GdtTableStepSize = GdtTssTableSize; @@ -127,61 +132,6 @@ InitGdt
> ( }
>
> /**
> - 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);
> - if (!FeaturePcdGet (PcdCpuSmmStackGuard)) {
> - //
> - // Do not set RO for IA32 when stack guard feature is enabled.
> - // Stack Guard need use task switch to switch stack.
> - // It need write GDT and TSS.
> - //
> - 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
> - );
> -}
> -
> -/**
> Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch.
>
> @param[in] ApHltLoopCode The address of the safe hlt-loop
> function.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index ef32f1767665..cbaa513244d5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1,7 +1,7 @@
> /** @file
> Agent Module to load other modules to deploy SMM Entry Vector for X86 CPU.
>
> -Copyright (c) 2009 - 2017, Intel Corporation. All rights
> reserved.<BR>
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights
> +reserved.<BR>
> Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>
> This program and the accompanying materials @@ -510,14 +510,6 @@
> InitGdt (
> );
>
> /**
> - This function sets GDT/IDT buffer to be RO and XP.
> -**/
> -VOID
> -PatchGdtIdtMap (
> - VOID
> - );
> -
> -/**
>
> Register the SMM Foundation entry point.
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 2d7dba59bf30..16664f304cde 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -1,6 +1,6 @@
> /** @file
>
> -Copyright (c) 2016 - 2017, Intel Corporation. All rights
> reserved.<BR>
> +Copyright (c) 2016 - 2018, Intel Corporation. All rights
> +reserved.<BR>
> This program and the accompanying materials are licensed and made
> available under the terms and conditions of the BSD License which
> accompanies this distribution. The full text of the license may be
> found at @@ -769,6 +769,53 @@ 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); // // The range
> + should have been set to RO // if it is allocated with
> + EfiRuntimeServicesCode.
> + //
> + 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 9d26e44a9acd..6a5d453242ff 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> @@ -1,7 +1,7 @@
> /** @file
> SMM CPU misc functions for x64 arch specific.
>
> -Copyright (c) 2015 - 2016, Intel Corporation. All rights
> reserved.<BR>
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights
> +reserved.<BR>
> This program and the accompanying materials are licensed and made
> available under the terms and conditions of the BSD License which
> accompanies this distribution. The full text of the license may be
> found at @@ -96,54 +96,6 @@ InitGdt ( }
>
> /**
> - 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
> - );
> -}
> -
> -/**
> Get Protected mode code segment from current GDT table.
>
> @return Protected mode code segment value.
> --
> 2.7.0.windows.1
prev parent reply other threads:[~2018-01-12 7:54 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-12 7:30 [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 Star Zeng
2018-01-12 7:59 ` Yao, Jiewen
2018-01-12 8:00 ` Zeng, Star [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=0C09AFA07DD0434D9E2A0C6AEB0483103B9FBB23@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