[edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer
Michael D Kinney
michael.d.kinney at intel.com
Fri Nov 5 21:39:42 UTC 2021
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 at ispras.ru>
> Sent: Friday, November 5, 2021 1:36 PM
> To: Kinney, Michael D <michael.d.kinney at intel.com>
> Cc: Leif Lindholm <leif at nuviainc.com>; devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>; Dong, Eric
> <eric.dong at intel.com>; Wang, Jian J <jian.j.wang at intel.com>; Jeff Fan <vanjeff_919 at hotmail.com>; Mikhail Krichanov
> <krichanov at ispras.ru>; Marvin Häuser <mhaeuser at 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 at 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 at nuviainc.com>
> >> Sent: Friday, November 5, 2021 12:28 PM
> >> To: devel at edk2.groups.io; cheptsov at ispras.ru
> >> Cc: Yao, Jiewen <jiewen.yao at intel.com>; Dong, Eric <eric.dong at intel.com>; Kinney, Michael D
> <michael.d.kinney at intel.com>;
> >> Wang, Jian J <jian.j.wang at intel.com>; Jeff Fan <vanjeff_919 at hotmail.com>; Mikhail Krichanov <krichanov at ispras.ru>;
> Marvin
> >> Häuser <mhaeuser at 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 at intel.com>
> >>>
> >>> Cc: Eric Dong <eric.dong at intel.com>
> >>>
> >>> Cc: Michael Kinney <michael.d.kinney at intel.com>
> >>>
> >>> Cc: Jian J Wang <jian.j.wang at intel.com>
> >>>
> >>> Cc: Jeff Fan <vanjeff_919 at hotmail.com>
> >>>
> >>> Cc: Mikhail Krichanov <krichanov at ispras.ru>
> >>>
> >>> Cc: Marvin Häuser <mhaeuser at posteo.de>
> >>>
> >>> Signed-off-by: Vitaly Cheptsov <cheptsov at 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)
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83412): https://edk2.groups.io/g/devel/message/83412
Mute This Topic: https://groups.io/mt/85741694/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list