[PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer


Vitaly Cheptsov
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3639

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jeff Fan <vanjeff_919@hotmail.com>
Cc: Mikhail Krichanov <krichanov@ispras.ru>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru>
---
.../Library/CpuExceptionHandlerLib/DxeException.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
index fd59f09ecd..12874811e1 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
@@ -22,7 +22,7 @@ EXCEPTION_HANDLER_DATA mExceptionHandlerData;

UINT8 mNewStack[CPU_STACK_SWITCH_EXCEPTION_NUMBER *
CPU_KNOWN_GOOD_STACK_SIZE];
-UINT8 mNewGdt[CPU_TSS_GDT_SIZE];
+UINT8 mNewGdt[CPU_TSS_GDT_SIZE + IA32_GDT_ALIGNMENT];

/**
Common exception handler.
@@ -238,6 +238,7 @@ InitializeCpuExceptionHandlersEx (
CPU_EXCEPTION_INIT_DATA EssData;
IA32_DESCRIPTOR Idtr;
IA32_DESCRIPTOR Gdtr;
+ UINT8 *Gdt;

//
// To avoid repeat initialization of default handlers, the caller should pass
@@ -259,6 +260,7 @@ InitializeCpuExceptionHandlersEx (
if (PcdGetBool (PcdCpuStackGuard)) {
if (InitData == NULL) {
SetMem (mNewGdt, sizeof (mNewGdt), 0);
+ Gdt = ALIGN_POINTER (mNewGdt, IA32_GDT_ALIGNMENT);

AsmReadIdtr (&Idtr);
AsmReadGdtr (&Gdtr);
@@ -270,11 +272,11 @@ InitializeCpuExceptionHandlersEx (
EssData.X64.StackSwitchExceptionNumber = CPU_STACK_SWITCH_EXCEPTION_NUMBER;
EssData.X64.IdtTable = (VOID *)Idtr.Base;
EssData.X64.IdtTableSize = Idtr.Limit + 1;
- EssData.X64.GdtTable = mNewGdt;
- EssData.X64.GdtTableSize = sizeof (mNewGdt);
- EssData.X64.ExceptionTssDesc = mNewGdt + Gdtr.Limit + 1;
+ EssData.X64.GdtTable = Gdt;
+ EssData.X64.GdtTableSize = CPU_TSS_GDT_SIZE;
+ EssData.X64.ExceptionTssDesc = Gdt + Gdtr.Limit + 1;
EssData.X64.ExceptionTssDescSize = CPU_TSS_DESC_SIZE;
- EssData.X64.ExceptionTss = mNewGdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
+ EssData.X64.ExceptionTss = Gdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
EssData.X64.ExceptionTssSize = CPU_TSS_SIZE;

InitData = &EssData;
--
2.30.1 (Apple Git-130)


Vitaly Cheptsov
 

Just to make it clear, this is an immediate solution that is good enough to fix the bug. However, a more proper solution would be to introduce the _Alignas concept to EDK II. I would suggest the following macro in Base.h:

/**
  Enforce custom alignment for a variable definition.
  Similar to C11 alignas macro from stdalign.h, except it must be functional to support MSVC.

  @param  Alignment  Numeric alignment to require.
**/
#ifdef _MSC_EXTENSIONS
  #define ALIGNAS(Alignment) __declspec(align(Alignment))
#else
  #define ALIGNAS(Alignment) _Alignas(Alignment)
#endif

If there is no disagreement on this, I can imagine submitting an update after this patch is merged.



Vitaly Cheptsov
 

Hello,

It has been over a month since the patch was sent. What is the state of the issue?

Best regards,
Vitaly

On 20 Sep 2021, at 19:15, vit9696 via [] <vit9696=protonmail.com@[]> wrote:

Just to make it clear, this is an immediate solution that is good enough to fix the bug. However, a more proper solution would be to introduce the _Alignas concept to EDK II. I would suggest the following macro in Base.h:

/**
Enforce custom alignment for a variable definition.
Similar to C11 alignas macro from stdalign.h, except it must be functional to support MSVC.

@param Alignment Numeric alignment to require.
**/
#ifdef _MSC_EXTENSIONS
#define ALIGNAS(Alignment) __declspec(align(Alignment))
#else
#define ALIGNAS(Alignment) _Alignas(Alignment)
#endif

If there is no disagreement on this, I can imagine submitting an update after this patch is merged.


REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3639

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jeff Fan <vanjeff_919@hotmail.com>
Cc: Mikhail Krichanov <krichanov@ispras.ru>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru>
---
.../Library/CpuExceptionHandlerLib/DxeException.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
index fd59f09ecd..12874811e1 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
@@ -22,7 +22,7 @@ EXCEPTION_HANDLER_DATA mExceptionHandlerData;

UINT8 mNewStack[CPU_STACK_SWITCH_EXCEPTION_NUMBER *
CPU_KNOWN_GOOD_STACK_SIZE];
-UINT8 mNewGdt[CPU_TSS_GDT_SIZE];
+UINT8 mNewGdt[CPU_TSS_GDT_SIZE + IA32_GDT_ALIGNMENT];

/**
Common exception handler.
@@ -238,6 +238,7 @@ InitializeCpuExceptionHandlersEx (
CPU_EXCEPTION_INIT_DATA EssData;
IA32_DESCRIPTOR Idtr;
IA32_DESCRIPTOR Gdtr;
+ UINT8 *Gdt;

//
// To avoid repeat initialization of default handlers, the caller should pass
@@ -259,6 +260,7 @@ InitializeCpuExceptionHandlersEx (
if (PcdGetBool (PcdCpuStackGuard)) {
if (InitData == NULL) {
SetMem (mNewGdt, sizeof (mNewGdt), 0);
+ Gdt = ALIGN_POINTER (mNewGdt, IA32_GDT_ALIGNMENT);

AsmReadIdtr (&Idtr);
AsmReadGdtr (&Gdtr);
@@ -270,11 +272,11 @@ InitializeCpuExceptionHandlersEx (
EssData.X64.StackSwitchExceptionNumber = CPU_STACK_SWITCH_EXCEPTION_NUMBER;
EssData.X64.IdtTable = (VOID *)Idtr.Base;
EssData.X64.IdtTableSize = Idtr.Limit + 1;
- EssData.X64.GdtTable = mNewGdt;
- EssData.X64.GdtTableSize = sizeof (mNewGdt);
- EssData.X64.ExceptionTssDesc = mNewGdt + Gdtr.Limit + 1;
+ EssData.X64.GdtTable = Gdt;
+ EssData.X64.GdtTableSize = CPU_TSS_GDT_SIZE;
+ EssData.X64.ExceptionTssDesc = Gdt + Gdtr.Limit + 1;
EssData.X64.ExceptionTssDescSize = CPU_TSS_DESC_SIZE;
- EssData.X64.ExceptionTss = mNewGdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
+ EssData.X64.ExceptionTss = Gdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
EssData.X64.ExceptionTssSize = CPU_TSS_SIZE;

InitData = &EssData;