From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by mx.groups.io with SMTP id smtpd.web12.15561.1636173753635719002 for ; Fri, 05 Nov 2021 21:42:34 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.84, mailfrom: cheptsov@ispras.ru) Received: from smtpclient.apple (unknown [77.232.9.83]) by mail.ispras.ru (Postfix) with ESMTPSA id 3B81340755C4; Sat, 6 Nov 2021 04:42:29 +0000 (UTC) Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.20.0.1.32\)) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer From: "Vitaly Cheptsov" In-Reply-To: Date: Sat, 6 Nov 2021 07:42:28 +0300 Cc: Leif Lindholm , "devel@edk2.groups.io" , "Yao, Jiewen" , "Dong, Eric" , "Wang, Jian J" , Jeff Fan , Mikhail Krichanov , =?utf-8?Q?Marvin_H=C3=A4user?= Message-Id: <3CA6752A-3CD9-42DD-B079-F9A273083294@ispras.ru> References: <20210920141347.25161-1-cheptsov@ispras.ru> <20211105192821.s2itdxh5t6azp4z6@leviathan> To: "Kinney, Michael D" X-Mailer: Apple Mail (2.3693.20.0.1.32) X-Groupsio-MsgNum: 83413 Content-Type: multipart/signed; boundary="Apple-Mail=_8B2BC136-5B38-400D-89C5-1488EE1C0618"; protocol="application/pgp-signature"; micalg=pgp-sha256 Content-Transfer-Encoding: quoted-printable --Apple-Mail=_8B2BC136-5B38-400D-89C5-1488EE1C0618 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 c= hoice. Best wishes, Vitaly [1] https://edk2.groups.io/g/devel/message/78887?p=3D%2C%2C%2C100%2C0%2C0%2= C0%3A%3Arecentpostdate%2Fsticky%2C%2CALIGNOF%2C100%2C2%2C0%2C84754061 > On 6 Nov 2021, at 00:39, Kinney, Michael D w= rote: >=20 > Hi Vitaly, >=20 > 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. >=20 > I would expect the X64 CLANGPDB build to use an 8-byte boundary. >=20 > 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. >=20 > 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; >=20 > IA32_SEGMENT_DESCRIPTOR mNewGdt[(CPU_TSS_GDT_SIZE / sizeof (IA32_SEGMENT= _DESCRIPTOR)) + 1]; >=20 > Add 1 in case CPU_TSS_GDT_SIZE is not 8-byte aligned in size. >=20 >=20 > 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. >=20 > I think in other places, the GDT is allocated using AllocatePool() > which guarantees an 8-byte alignment. >=20 > 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. >=20 > Mike >=20 >> -----Original Message----- >> From: Vitaly Cheptsov >> Sent: Friday, November 5, 2021 1:36 PM >> To: Kinney, Michael D >> Cc: Leif Lindholm ; devel@edk2.groups.io; Yao, Jiewen= ; Dong, Eric >> ; Wang, Jian J ; Jeff Fan ; Mikhail Krichanov >> ; Marvin H=C3=A4user >> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard suppor= t by aligning GDT buffer >>=20 >> Hi Mike, >>=20 >> The command is: >> build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t CLANGPDB -b NOOPT -D DEBUG_O= N_SERIAL_PORT >>=20 >> But I obviously needed to add >> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|TRUE >> to OvmfPkgIa32.dsc. >>=20 >> The compiler is clang 12.0.1, because clang 13 is badly broken[1]. It wo= uld then crash like this: >>=20 >> HandOffToDxeCore() Stack Base: 0x7FE24000, Stack Size: 0x20000 >>=20 >> ASSERT_EFI_ERROR (Status =3D Invalid Parameter) >> ASSERT [DxeCore] DxeMain.c(256): !EFI_ERROR (Status) >>=20 >> As for aligning the array, I can submit a V2 which introduces the ALIGNA= S macro. I also like this solution a lot more. >>=20 >> Best wishes, >> Vitaly >>=20 >> [1] https://bugzilla.tianocore.org/buglist.cgi?quicksearch=3Dclang%2013&= list_id=3D24276 >>=20 >>=20 >>> On 5 Nov 2021, at 22:42, Kinney, Michael D = wrote: >>>=20 >>> Hi Vitaly, >>>=20 >>> Can you please provide some details on the compiler/build command that = did not align the array >>> correctly. >>>=20 >>> I agree that the GDT must have the correct alignment. >>>=20 >>> 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. >>>=20 >>>=20 >>> Mike >>>=20 >>>> -----Original Message----- >>>> From: Leif Lindholm >>>> Sent: Friday, November 5, 2021 12:28 PM >>>> To: devel@edk2.groups.io; cheptsov@ispras.ru >>>> Cc: Yao, Jiewen ; Dong, Eric ; Kinney, Michael D >> ; >>>> Wang, Jian J ; Jeff Fan ; Mikhail Krichanov ; >> Marvin >>>> H=C3=A4user >>>> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard supp= ort by aligning GDT buffer >>>>=20 >>>> UefiCpuPkg maintainers - please respond. >>>>=20 >>>> Meanwhile, Vitaly, could you please provide a commit message? >>>> The BZ link is needed, but it's not a substitute. >>>>=20 >>>> / >>>> Leif >>>>=20 >>>> On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3639 >>>>>=20 >>>>>=20 >>>>>=20 >>>>> Cc: Jiewen Yao >>>>>=20 >>>>> Cc: Eric Dong >>>>>=20 >>>>> Cc: Michael Kinney >>>>>=20 >>>>> Cc: Jian J Wang >>>>>=20 >>>>> Cc: Jeff Fan >>>>>=20 >>>>> Cc: Mikhail Krichanov >>>>>=20 >>>>> Cc: Marvin H=C3=A4user >>>>>=20 >>>>> Signed-off-by: Vitaly Cheptsov >>>>>=20 >>>>> --- >>>>>=20 >>>>> .../Library/CpuExceptionHandlerLib/DxeException.c | 12 +++++++----= - >>>>>=20 >>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>>>=20 >>>>>=20 >>>>>=20 >>>>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>>>=20 >>>>> index fd59f09ecd..12874811e1 100644 >>>>>=20 >>>>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>>>=20 >>>>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>>>=20 >>>>> @@ -22,7 +22,7 @@ EXCEPTION_HANDLER_DATA mExceptionHandlerData; >>>>>=20 >>>>>=20 >>>>>=20 >>>>> UINT8 mNewStack[CPU_STACK_SWITCH_EXCEPTION_NUMB= ER * >>>>>=20 >>>>> CPU_KNOWN_GOOD_STACK_SIZE]; >>>>>=20 >>>>> -UINT8 mNewGdt[CPU_TSS_GDT_SIZE]; >>>>>=20 >>>>> +UINT8 mNewGdt[CPU_TSS_GDT_SIZE + IA32_GDT_ALIG= NMENT]; >>>>>=20 >>>>>=20 >>>>>=20 >>>>> /** >>>>>=20 >>>>> Common exception handler. >>>>>=20 >>>>> @@ -238,6 +238,7 @@ InitializeCpuExceptionHandlersEx ( >>>>>=20 >>>>> CPU_EXCEPTION_INIT_DATA EssData; >>>>>=20 >>>>> IA32_DESCRIPTOR Idtr; >>>>>=20 >>>>> IA32_DESCRIPTOR Gdtr; >>>>>=20 >>>>> + UINT8 *Gdt; >>>>>=20 >>>>>=20 >>>>>=20 >>>>> // >>>>>=20 >>>>> // To avoid repeat initialization of default handlers, the caller sh= ould pass >>>>>=20 >>>>> @@ -259,6 +260,7 @@ InitializeCpuExceptionHandlersEx ( >>>>>=20 >>>>> if (PcdGetBool (PcdCpuStackGuard)) { >>>>>=20 >>>>> if (InitData =3D=3D NULL) { >>>>>=20 >>>>> SetMem (mNewGdt, sizeof (mNewGdt), 0); >>>>>=20 >>>>> + Gdt =3D ALIGN_POINTER (mNewGdt, IA32_GDT_ALIGNMENT); >>>>>=20 >>>>>=20 >>>>>=20 >>>>> AsmReadIdtr (&Idtr); >>>>>=20 >>>>> AsmReadGdtr (&Gdtr); >>>>>=20 >>>>> @@ -270,11 +272,11 @@ InitializeCpuExceptionHandlersEx ( >>>>>=20 >>>>> EssData.X64.StackSwitchExceptionNumber =3D CPU_STACK_SWITCH_EX= CEPTION_NUMBER; >>>>>=20 >>>>> EssData.X64.IdtTable =3D (VOID *)Idtr.Base; >>>>>=20 >>>>> EssData.X64.IdtTableSize =3D Idtr.Limit + 1; >>>>>=20 >>>>> - EssData.X64.GdtTable =3D mNewGdt; >>>>>=20 >>>>> - EssData.X64.GdtTableSize =3D sizeof (mNewGdt); >>>>>=20 >>>>> - EssData.X64.ExceptionTssDesc =3D mNewGdt + Gdtr.Limit + 1; >>>>>=20 >>>>> + EssData.X64.GdtTable =3D Gdt; >>>>>=20 >>>>> + EssData.X64.GdtTableSize =3D CPU_TSS_GDT_SIZE; >>>>>=20 >>>>> + EssData.X64.ExceptionTssDesc =3D Gdt + Gdtr.Limit + 1; >>>>>=20 >>>>> EssData.X64.ExceptionTssDescSize =3D CPU_TSS_DESC_SIZE; >>>>>=20 >>>>> - EssData.X64.ExceptionTss =3D mNewGdt + Gdtr.Limit + 1 + CPU_= TSS_DESC_SIZE; >>>>>=20 >>>>> + EssData.X64.ExceptionTss =3D Gdt + Gdtr.Limit + 1 + CPU_TSS_= DESC_SIZE; >>>>>=20 >>>>> EssData.X64.ExceptionTssSize =3D CPU_TSS_SIZE; >>>>>=20 >>>>>=20 >>>>>=20 >>>>> InitData =3D &EssData; >>>>>=20 >>>>> -- >>>>>=20 >>>>> 2.30.1 (Apple Git-130) >>>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >=20 --Apple-Mail=_8B2BC136-5B38-400D-89C5-1488EE1C0618 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAmGGB7QACgkQL8K2O86E yz5GVA//ZCiTErndHuVRUXeXVGpKKeoZ9dubEAUNYZQwELWd/VS2GZY54gpVD9BQ d5P8rri1RZBY0dRhzMR/IPzXg+yIXU79QHBrJp65CWh+hwG3o69qWtlC7mDfSk4d x55TSkbigy1Mv6DOdqmQVS/AIlaN7DpgSxcevkKTQiFE59LcaQ1p4VPe2bsIg/+6 pS1KK3wNQx+WHayapoxGMqWz6gGq2U0cb4EoBdUWTy5IFo/ihAV4xV5lDdcccC5h 4P/NxHi+LML6q8KD5qxZN8bKyq5hBOnJw6piM9P1u9lvbZb+OQLI+xiwz9/0/fXw /rAutDtwBxEEIi+vsi8+alVbJ1ZPwMwJqpC+6bdEZ5K+8HXRgfRG2Cz4YJTfSFdp 6n3JwOw8WjZCFV39HO8kEneI7vj3wQ2RbBvbM6MaP9tAU7OqpIOJAwXVt2VJSzJp qzbl3YwICBdRJGpXpSZXFF+TzIciHHNAkDMNJN3MOgqxIfU+EaPDUtqsR0hsJGPY w3+ZeBG3WOHkprv7Kzd47lXFPfbQ8CC6MeD8x2LNWtiaJGhj91vqzeokQlx8B7vM jTCsX8BPCePYELUUQsTMZuSOJdy+RB4h3IdX2R1lnIdlDVP7oYwMMtnH4yFCZgdh f4ZsS8tJKFhbYtc2YQgGdXKz0ihuTfe5+ohT3GCwX4ZTzwErW2g= =Qjab -----END PGP SIGNATURE----- --Apple-Mail=_8B2BC136-5B38-400D-89C5-1488EE1C0618--