* [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer @ 2021-09-20 14:13 Vitaly Cheptsov 2021-09-20 16:15 ` [edk2-devel] " Vitaly Cheptsov 2021-11-05 19:28 ` Leif Lindholm 0 siblings, 2 replies; 10+ messages in thread From: Vitaly Cheptsov @ 2021-09-20 14:13 UTC (permalink / raw) To: devel Cc: Jiewen Yao, Eric Dong, Michael Kinney, Jian J Wang, Jeff Fan, Mikhail Krichanov, Marvin Häuser 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) ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-09-20 14:13 [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer Vitaly Cheptsov @ 2021-09-20 16:15 ` Vitaly Cheptsov 2021-10-24 10:59 ` Vitaly Cheptsov 2021-11-05 19:28 ` Leif Lindholm 1 sibling, 1 reply; 10+ messages in thread From: Vitaly Cheptsov @ 2021-09-20 16:15 UTC (permalink / raw) To: Vitaly Cheptsov, devel [-- Attachment #1: Type: text/plain, Size: 736 bytes --] 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 ( https://en.cppreference.com/w/c/language/_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. [-- Attachment #2: Type: text/html, Size: 969 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-09-20 16:15 ` [edk2-devel] " Vitaly Cheptsov @ 2021-10-24 10:59 ` Vitaly Cheptsov 0 siblings, 0 replies; 10+ messages in thread From: Vitaly Cheptsov @ 2021-10-24 10:59 UTC (permalink / raw) To: devel Cc: Jiewen Yao, Eric Dong, Michael Kinney, Jian J Wang, Jeff Fan, Mikhail Krichanov, Marvin Häuser [-- Attachment #1: Type: text/plain, Size: 3848 bytes --] 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; > [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-09-20 14:13 [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer Vitaly Cheptsov 2021-09-20 16:15 ` [edk2-devel] " Vitaly Cheptsov @ 2021-11-05 19:28 ` Leif Lindholm 2021-11-05 19:37 ` Vitaly Cheptsov 2021-11-05 19:42 ` Michael D Kinney 1 sibling, 2 replies; 10+ messages in thread From: Leif Lindholm @ 2021-11-05 19:28 UTC (permalink / raw) To: devel, cheptsov Cc: Jiewen Yao, Eric Dong, Michael Kinney, Jian J Wang, Jeff Fan, Mikhail Krichanov, Marvin Häuser UefiCpuPkg maintainers - please respond. Meanwhile, Vitaly, could you please provide a commit message? The BZ link is needed, but it's not a substitute. / Leif On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: > 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) > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-11-05 19:28 ` Leif Lindholm @ 2021-11-05 19:37 ` Vitaly Cheptsov 2021-11-08 14:04 ` Leif Lindholm 2021-11-05 19:42 ` Michael D Kinney 1 sibling, 1 reply; 10+ messages in thread From: Vitaly Cheptsov @ 2021-11-05 19:37 UTC (permalink / raw) To: Leif Lindholm Cc: devel, Jiewen Yao, Eric Dong, Michael Kinney, Jian J Wang, Jeff Fan, Mikhail Krichanov, Marvin Häuser [-- Attachment #1: Type: text/plain, Size: 4231 bytes --] Hi Leif, I assume you mean the commit description, because the commit message is in the topic. I believe something like that would do: CpuExceptionHandlerLib supplies misaligned GDT to the outer world (e.g. ArchSetupExceptionStack) when PcdCpuStackGuard is enabled. This happens because it uses an array of UINT8 for the mNewGdt variable, which alignment is 1 byte versus required 8 bytes. As a result ArchSetupExceptionStack always returns EFI_INVALID_PARAMETER in OVMF Ia32 with XCODE5 and CLANGPDB at least. Fix this by allocating extra space in mNewGdt and then aligning the pointer upwards. Best wishes, Vitaly > On 5 Nov 2021, at 22:28, Leif Lindholm <leif@nuviainc.com> wrote: > > UefiCpuPkg maintainers - please respond. > > Meanwhile, Vitaly, could you please provide a commit message? > The BZ link is needed, but it's not a substitute. > > / > Leif > > On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: >> 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) >> >> >> >> >> >> >> >> [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-11-05 19:37 ` Vitaly Cheptsov @ 2021-11-08 14:04 ` Leif Lindholm 0 siblings, 0 replies; 10+ messages in thread From: Leif Lindholm @ 2021-11-08 14:04 UTC (permalink / raw) To: Vitaly Cheptsov Cc: edk2-devel-groups-io, Jiewen Yao, Eric Dong, Michael Kinney, Jian J Wang, Jeff Fan, Mikhail Krichanov, Marvin Häuser [-- Attachment #1: Type: text/plain, Size: 4675 bytes --] On Fri, Nov 5, 2021 at 7:37 PM Vitaly Cheptsov <cheptsov@ispras.ru> wrote: > > Hi Leif, > > I assume you mean the commit description, because the commit message is in the topic. A topic is not a commit message. The commit message is what comes after the topic. > I believe something like that would do: > > CpuExceptionHandlerLib supplies misaligned GDT to the outer world > (e.g. ArchSetupExceptionStack) when PcdCpuStackGuard is enabled. > This happens because it uses an array of UINT8 for the mNewGdt > variable, which alignment is 1 byte versus required 8 bytes. As a result > ArchSetupExceptionStack always returns EFI_INVALID_PARAMETER in OVMF Ia32 > with XCODE5 and CLANGPDB at least. > > Fix this by allocating extra space in mNewGdt and then aligning the pointer > upwards. But I'm happy with this one. Best Regards, Leif > Best wishes, > Vitaly > > > On 5 Nov 2021, at 22:28, Leif Lindholm <leif@nuviainc.com> wrote: > > > > UefiCpuPkg maintainers - please respond. > > > > Meanwhile, Vitaly, could you please provide a commit message? > > The BZ link is needed, but it's not a substitute. > > > > / > > Leif > > > > On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: > >> 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) > >> > >> > >> > >> > >> > >> > >> > >> [-- Attachment #2: Type: text/html, Size: 7151 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-11-05 19:28 ` Leif Lindholm 2021-11-05 19:37 ` Vitaly Cheptsov @ 2021-11-05 19:42 ` Michael D Kinney 2021-11-05 20:36 ` Vitaly Cheptsov 1 sibling, 1 reply; 10+ messages in thread From: Michael D Kinney @ 2021-11-05 19:42 UTC (permalink / raw) To: Leif Lindholm, devel@edk2.groups.io, cheptsov@ispras.ru, Kinney, Michael D Cc: Yao, Jiewen, Dong, Eric, Wang, Jian J, Jeff Fan, Mikhail Krichanov, Marvin Häuser Hi Vitaly, Can you please provide some details on the compiler/build command that did not align the array correctly. I agree that the GDT must have the correct alignment. I do not like the idea of unused bytes at the beginning of the array. I would prefer to see an array that is aligned correctly by declaration. Mike > -----Original Message----- > From: Leif Lindholm <leif@nuviainc.com> > Sent: Friday, November 5, 2021 12:28 PM > To: devel@edk2.groups.io; cheptsov@ispras.ru > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; > Wang, Jian J <jian.j.wang@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>; Mikhail Krichanov <krichanov@ispras.ru>; Marvin > Häuser <mhaeuser@posteo.de> > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer > > UefiCpuPkg maintainers - please respond. > > Meanwhile, Vitaly, could you please provide a commit message? > The BZ link is needed, but it's not a substitute. > > / > Leif > > On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: > > 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) > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-11-05 19:42 ` Michael D Kinney @ 2021-11-05 20:36 ` Vitaly Cheptsov 2021-11-05 21:39 ` Michael D Kinney 0 siblings, 1 reply; 10+ messages in thread From: Vitaly Cheptsov @ 2021-11-05 20:36 UTC (permalink / raw) To: Kinney, Michael D Cc: Leif Lindholm, devel@edk2.groups.io, Yao, Jiewen, Dong, Eric, Wang, Jian J, Jeff Fan, Mikhail Krichanov, Marvin Häuser [-- Attachment #1: Type: text/plain, Size: 5402 bytes --] Hi Mike, The command is: build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t CLANGPDB -b NOOPT -D DEBUG_ON_SERIAL_PORT But I obviously needed to add gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|TRUE to OvmfPkgIa32.dsc. The compiler is clang 12.0.1, because clang 13 is badly broken[1]. It would then crash like this: HandOffToDxeCore() Stack Base: 0x7FE24000, Stack Size: 0x20000 ASSERT_EFI_ERROR (Status = Invalid Parameter) ASSERT [DxeCore] DxeMain.c(256): !EFI_ERROR (Status) As for aligning the array, I can submit a V2 which introduces the ALIGNAS macro. I also like this solution a lot more. Best wishes, Vitaly [1] https://bugzilla.tianocore.org/buglist.cgi?quicksearch=clang%2013&list_id=24276 > On 5 Nov 2021, at 22:42, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > Hi Vitaly, > > Can you please provide some details on the compiler/build command that did not align the array > correctly. > > I agree that the GDT must have the correct alignment. > > I do not like the idea of unused bytes at the beginning of the array. I would prefer to see > an array that is aligned correctly by declaration. > > > Mike > >> -----Original Message----- >> From: Leif Lindholm <leif@nuviainc.com> >> Sent: Friday, November 5, 2021 12:28 PM >> To: devel@edk2.groups.io; cheptsov@ispras.ru >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; >> Wang, Jian J <jian.j.wang@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>; Mikhail Krichanov <krichanov@ispras.ru>; Marvin >> Häuser <mhaeuser@posteo.de> >> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer >> >> UefiCpuPkg maintainers - please respond. >> >> Meanwhile, Vitaly, could you please provide a commit message? >> The BZ link is needed, but it's not a substitute. >> >> / >> Leif >> >> On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: >>> 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) >>> >>> >>> >>> >>> >>> >>> >>> [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-11-05 20:36 ` Vitaly Cheptsov @ 2021-11-05 21:39 ` Michael D Kinney 2021-11-06 4:42 ` Vitaly Cheptsov 0 siblings, 1 reply; 10+ messages in thread From: Michael D Kinney @ 2021-11-05 21:39 UTC (permalink / raw) To: Vitaly Cheptsov, Kinney, Michael D Cc: Leif Lindholm, devel@edk2.groups.io, Yao, Jiewen, Dong, Eric, Wang, Jian J, Jeff Fan, Mikhail Krichanov, Marvin Häuser Hi Vitaly, So IA32 CLANGPDB is likely putting the UINTT8 array global mNewGdt on a 4-byte boundary, when it would have to be on an 8-byte boundary to meet the GDT alignment requirements. I would expect the X64 CLANGPDB build to use an 8-byte boundary. Another option to align to 8-byte boundary is to make the array an array of UINT64 values. To be more correct, we could use an array of IA32_SEGMENT_DESCRIPTOR structures that is a union with a UINT64 to guarantee 8-byte alignment. typedef union { struct { UINT32 LimitLow:16; UINT32 BaseLow:16; UINT32 BaseMid:8; UINT32 Type:4; UINT32 S:1; UINT32 DPL:2; UINT32 P:1; UINT32 LimitHigh:4; UINT32 AVL:1; UINT32 L:1; UINT32 DB:1; UINT32 G:1; UINT32 BaseHigh:8; } Bits; UINT64 Uint64; } IA32_SEGMENT_DESCRIPTOR; IA32_SEGMENT_DESCRIPTOR mNewGdt[(CPU_TSS_GDT_SIZE / sizeof (IA32_SEGMENT_DESCRIPTOR)) + 1]; Add 1 in case CPU_TSS_GDT_SIZE is not 8-byte aligned in size. For come of the logic, you would still need to introduce a UINT8 * local variable to compute the start of each component in this array. I think in other places, the GDT is allocated using AllocatePool() which guarantees an 8-byte alignment. Are you suggesting that the ALIGNAS macro would map to the compiler specific alignment directives? We have not had to introduce that yet and would like to figure out how to address this one issues without adding those directives. Mike > -----Original Message----- > From: Vitaly Cheptsov <cheptsov@ispras.ru> > Sent: Friday, November 5, 2021 1:36 PM > To: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric > <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>; Mikhail Krichanov > <krichanov@ispras.ru>; Marvin Häuser <mhaeuser@posteo.de> > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer > > Hi Mike, > > The command is: > build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t CLANGPDB -b NOOPT -D DEBUG_ON_SERIAL_PORT > > But I obviously needed to add > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|TRUE > to OvmfPkgIa32.dsc. > > The compiler is clang 12.0.1, because clang 13 is badly broken[1]. It would then crash like this: > > HandOffToDxeCore() Stack Base: 0x7FE24000, Stack Size: 0x20000 > > ASSERT_EFI_ERROR (Status = Invalid Parameter) > ASSERT [DxeCore] DxeMain.c(256): !EFI_ERROR (Status) > > As for aligning the array, I can submit a V2 which introduces the ALIGNAS macro. I also like this solution a lot more. > > Best wishes, > Vitaly > > [1] https://bugzilla.tianocore.org/buglist.cgi?quicksearch=clang%2013&list_id=24276 > > > > On 5 Nov 2021, at 22:42, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > > > Hi Vitaly, > > > > Can you please provide some details on the compiler/build command that did not align the array > > correctly. > > > > I agree that the GDT must have the correct alignment. > > > > I do not like the idea of unused bytes at the beginning of the array. I would prefer to see > > an array that is aligned correctly by declaration. > > > > > > Mike > > > >> -----Original Message----- > >> From: Leif Lindholm <leif@nuviainc.com> > >> Sent: Friday, November 5, 2021 12:28 PM > >> To: devel@edk2.groups.io; cheptsov@ispras.ru > >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; > >> Wang, Jian J <jian.j.wang@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>; Mikhail Krichanov <krichanov@ispras.ru>; > Marvin > >> Häuser <mhaeuser@posteo.de> > >> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer > >> > >> UefiCpuPkg maintainers - please respond. > >> > >> Meanwhile, Vitaly, could you please provide a commit message? > >> The BZ link is needed, but it's not a substitute. > >> > >> / > >> Leif > >> > >> On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: > >>> 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) > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >>> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer 2021-11-05 21:39 ` Michael D Kinney @ 2021-11-06 4:42 ` Vitaly Cheptsov 0 siblings, 0 replies; 10+ messages in thread From: Vitaly Cheptsov @ 2021-11-06 4:42 UTC (permalink / raw) To: Kinney, Michael D Cc: Leif Lindholm, devel@edk2.groups.io, Yao, Jiewen, Dong, Eric, Wang, Jian J, Jeff Fan, Mikhail Krichanov, Marvin Häuser [-- Attachment #1: Type: text/plain, Size: 8482 bytes --] Hi Mike, I am aware of this possibility, but it feels unnecessary ugly in my opinion. Marvin has already sent a patch alignment-related patches not so long ago[1], updating this with V3 and using as a ground feels like a much better choice. Best wishes, Vitaly [1] https://edk2.groups.io/g/devel/message/78887?p=%2C%2C%2C100%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CALIGNOF%2C100%2C2%2C0%2C84754061 > On 6 Nov 2021, at 00:39, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > Hi Vitaly, > > So IA32 CLANGPDB is likely putting the UINTT8 array global mNewGdt > on a 4-byte boundary, when it would have to be on an 8-byte boundary > to meet the GDT alignment requirements. > > I would expect the X64 CLANGPDB build to use an 8-byte boundary. > > Another option to align to 8-byte boundary is to make the array an > array of UINT64 values. To be more correct, we could use an array > of IA32_SEGMENT_DESCRIPTOR structures that is a union with a > UINT64 to guarantee 8-byte alignment. > > typedef union { > struct { > UINT32 LimitLow:16; > UINT32 BaseLow:16; > UINT32 BaseMid:8; > UINT32 Type:4; > UINT32 S:1; > UINT32 DPL:2; > UINT32 P:1; > UINT32 LimitHigh:4; > UINT32 AVL:1; > UINT32 L:1; > UINT32 DB:1; > UINT32 G:1; > UINT32 BaseHigh:8; > } Bits; > UINT64 Uint64; > } IA32_SEGMENT_DESCRIPTOR; > > IA32_SEGMENT_DESCRIPTOR mNewGdt[(CPU_TSS_GDT_SIZE / sizeof (IA32_SEGMENT_DESCRIPTOR)) + 1]; > > Add 1 in case CPU_TSS_GDT_SIZE is not 8-byte aligned in size. > > > For come of the logic, you would still need to introduce a UINT8 * local variable > to compute the start of each component in this array. > > I think in other places, the GDT is allocated using AllocatePool() > which guarantees an 8-byte alignment. > > Are you suggesting that the ALIGNAS macro would map to the compiler > specific alignment directives? We have not had to introduce that yet > and would like to figure out how to address this one issues without > adding those directives. > > Mike > >> -----Original Message----- >> From: Vitaly Cheptsov <cheptsov@ispras.ru> >> Sent: Friday, November 5, 2021 1:36 PM >> To: Kinney, Michael D <michael.d.kinney@intel.com> >> Cc: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric >> <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>; Mikhail Krichanov >> <krichanov@ispras.ru>; Marvin Häuser <mhaeuser@posteo.de> >> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer >> >> Hi Mike, >> >> The command is: >> build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t CLANGPDB -b NOOPT -D DEBUG_ON_SERIAL_PORT >> >> But I obviously needed to add >> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|TRUE >> to OvmfPkgIa32.dsc. >> >> The compiler is clang 12.0.1, because clang 13 is badly broken[1]. It would then crash like this: >> >> HandOffToDxeCore() Stack Base: 0x7FE24000, Stack Size: 0x20000 >> >> ASSERT_EFI_ERROR (Status = Invalid Parameter) >> ASSERT [DxeCore] DxeMain.c(256): !EFI_ERROR (Status) >> >> As for aligning the array, I can submit a V2 which introduces the ALIGNAS macro. I also like this solution a lot more. >> >> Best wishes, >> Vitaly >> >> [1] https://bugzilla.tianocore.org/buglist.cgi?quicksearch=clang%2013&list_id=24276 >> >> >>> On 5 Nov 2021, at 22:42, Kinney, Michael D <michael.d.kinney@intel.com> wrote: >>> >>> Hi Vitaly, >>> >>> Can you please provide some details on the compiler/build command that did not align the array >>> correctly. >>> >>> I agree that the GDT must have the correct alignment. >>> >>> I do not like the idea of unused bytes at the beginning of the array. I would prefer to see >>> an array that is aligned correctly by declaration. >>> >>> >>> Mike >>> >>>> -----Original Message----- >>>> From: Leif Lindholm <leif@nuviainc.com> >>>> Sent: Friday, November 5, 2021 12:28 PM >>>> To: devel@edk2.groups.io; cheptsov@ispras.ru >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; >>>> Wang, Jian J <jian.j.wang@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>; Mikhail Krichanov <krichanov@ispras.ru>; >> Marvin >>>> Häuser <mhaeuser@posteo.de> >>>> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer >>>> >>>> UefiCpuPkg maintainers - please respond. >>>> >>>> Meanwhile, Vitaly, could you please provide a commit message? >>>> The BZ link is needed, but it's not a substitute. >>>> >>>> / >>>> Leif >>>> >>>> On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: >>>>> 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) >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> > [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-08 14:05 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-20 14:13 [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer Vitaly Cheptsov 2021-09-20 16:15 ` [edk2-devel] " Vitaly Cheptsov 2021-10-24 10:59 ` Vitaly Cheptsov 2021-11-05 19:28 ` Leif Lindholm 2021-11-05 19:37 ` Vitaly Cheptsov 2021-11-08 14:04 ` Leif Lindholm 2021-11-05 19:42 ` Michael D Kinney 2021-11-05 20:36 ` Vitaly Cheptsov 2021-11-05 21:39 ` Michael D Kinney 2021-11-06 4:42 ` Vitaly Cheptsov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox