Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer

["Vitaly Cheptsov" <cheptsov@ispras.ru>]

[-- 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 --]