From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f180.google.com (mail-pf1-f180.google.com [209.85.210.180]) by mx.groups.io with SMTP id smtpd.web12.44994.1636380303600857774 for ; Mon, 08 Nov 2021 06:05:03 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20210112.gappssmtp.com header.s=20210112 header.b=qsfguaKa; spf=pass (domain: nuviainc.com, ip: 209.85.210.180, mailfrom: leif@nuviainc.com) Received: by mail-pf1-f180.google.com with SMTP id c126so7651642pfb.0 for ; Mon, 08 Nov 2021 06:05:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nHPBR7NvfI02jIN/Mtx0it4YR0YTOGcFWk6THTQcRD8=; b=qsfguaKaLO6NDHwuSQYRAwUVfzEhmeA/NYh92WMNF/guAZZGlfVsRJmUdfsllIDDyF 4uVLqMlUjovYtyJgzdrM1qputsx/dSehZuM0d43gmWm6u2rmAw3kXV733LxvVJVhdJwf XPwfaWjWdWe4K/9FoaUsgt5HOWiaYFqySSs/VX+nDCVOMladGhoJ1NVKS1eQcI6SnF/z k5hwACcx3RFx5m/fxk8NB8XFt+980gA3RL3rjmtxjyPu9kmoCkUkWoHh0tP/yLooXL7a u/RwVtB04GuK1RvAyPDuvBWkawqy1y4J7iwruNoAdf9ZkbKpMdpaWlbzVBr/oc3lI3Ny xL4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nHPBR7NvfI02jIN/Mtx0it4YR0YTOGcFWk6THTQcRD8=; b=4lsqww5rY6ZwW3moT7UdJo32qvlc5x9+avalRY4iJlAc4sZR6QokrC1P9fWWkuS6FH UUZMiMQkTwSJ3aPvsYfbTqkfHYJLl8M1IEAYNORCn+7cA1u5jeNb2EOWAt0eHbVRjHvJ uj7dIYNch1UW8BHgu8dcD3yCTOIy5p8FnGehaoszJRvL8CeIwypPiT5ZUcB7zbKmyDHm wPQWj5BjFW83kBrKfuEAN6GMxJ5Jlmzf7eqyoLuRd7YzK6qwORoA7r0waCGNJv4wAaQW tKsh1KX4Rxl2tWxvnW0jy3TaYaS8IMV9U4q7kEAIC889L3Z9nQ76VXynF5GCP3hS9/2x Ec0g== X-Gm-Message-State: AOAM531X1cA8NvsuHpyUcRdmtwFkA3PBxfwfQaRdheCP08Yptbl65sqw k6SwgMmkZHNkFjaFSUXR5ZrUIxWogZdwy1RuurkkyA== X-Google-Smtp-Source: ABdhPJx750YyRUoxBHz1IIgIDeH21ZkR2tgrWj5HnFHJEL7cJm1e/QLwW0EWsu0W3wuF1siqwe/QzFjpONiZRm0f6/0= X-Received: by 2002:a63:d605:: with SMTP id q5mr81508pgg.109.1636380302975; Mon, 08 Nov 2021 06:05:02 -0800 (PST) MIME-Version: 1.0 References: <20210920141347.25161-1-cheptsov@ispras.ru> <20211105192821.s2itdxh5t6azp4z6@leviathan> <081FC9DB-3E1C-4FAB-8F4F-CD48F85F1EC4@ispras.ru> In-Reply-To: <081FC9DB-3E1C-4FAB-8F4F-CD48F85F1EC4@ispras.ru> From: "Leif Lindholm" Date: Mon, 8 Nov 2021 14:04:52 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer To: Vitaly Cheptsov Cc: edk2-devel-groups-io , Jiewen Yao , Eric Dong , Michael Kinney , Jian J Wang , Jeff Fan , Mikhail Krichanov , =?UTF-8?Q?Marvin_H=C3=A4user?= Content-Type: multipart/alternative; boundary="000000000000ad3b8105d0477a35" --000000000000ad3b8105d0477a35 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Nov 5, 2021 at 7:37 PM Vitaly Cheptsov 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 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=3D3639 > >> > >> > >> > >> Cc: Jiewen Yao > >> > >> Cc: Eric Dong > >> > >> Cc: Michael Kinney > >> > >> Cc: Jian J Wang > >> > >> Cc: Jeff Fan > >> > >> Cc: Mikhail Krichanov > >> > >> Cc: Marvin H=C3=A4user > >> > >> Signed-off-by: Vitaly Cheptsov > >> > >> --- > >> > >> .../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 =3D=3D NULL) { > >> > >> SetMem (mNewGdt, sizeof (mNewGdt), 0); > >> > >> + Gdt =3D ALIGN_POINTER (mNewGdt, IA32_GDT_ALIGNMENT); > >> > >> > >> > >> AsmReadIdtr (&Idtr); > >> > >> AsmReadGdtr (&Gdtr); > >> > >> @@ -270,11 +272,11 @@ InitializeCpuExceptionHandlersEx ( > >> > >> EssData.X64.StackSwitchExceptionNumber =3D CPU_STACK_SWITCH_EXCEPTION_NUMBER; > >> > >> EssData.X64.IdtTable =3D (VOID *)Idtr.Base; > >> > >> EssData.X64.IdtTableSize =3D Idtr.Limit + 1; > >> > >> - EssData.X64.GdtTable =3D mNewGdt; > >> > >> - EssData.X64.GdtTableSize =3D sizeof (mNewGdt); > >> > >> - EssData.X64.ExceptionTssDesc =3D mNewGdt + Gdtr.Limit + 1; > >> > >> + EssData.X64.GdtTable =3D Gdt; > >> > >> + EssData.X64.GdtTableSize =3D CPU_TSS_GDT_SIZE; > >> > >> + EssData.X64.ExceptionTssDesc =3D Gdt + Gdtr.Limit + 1; > >> > >> EssData.X64.ExceptionTssDescSize =3D CPU_TSS_DESC_SIZE; > >> > >> - EssData.X64.ExceptionTss =3D mNewGdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE; > >> > >> + EssData.X64.ExceptionTss =3D Gdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE; > >> > >> EssData.X64.ExceptionTssSize =3D CPU_TSS_SIZE; > >> > >> > >> > >> InitData =3D &EssData; > >> > >> -- > >> > >> 2.30.1 (Apple Git-130) > >> > >> > >> > >> > >> > >>=20 > >> > >> --000000000000ad3b8105d0477a35 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Nov 5, 2021 at 7:37 PM Vitaly Cheptsov <= ;cheptsov@ispras.ru> wrote:>
> Hi Leif,
>
> I assume you mean the commit descrip= tion, because the commit message is in the topic.

A topi= c is not a commit message. The commit message is what comes after the topic= .

>=C2=A0 I believe something like that would d= o:
>
> CpuExceptionHandlerLib supplies misaligned GDT to the ou= ter world
> (e.g. ArchSetupExceptionStack) when PcdCpuStackGuard is e= nabled.
> This happens because it uses an array of UINT8 for the mNew= Gdt
> variable, which alignment is 1 byte versus required 8 bytes. As= a result
> ArchSetupExceptionStack always returns EFI_INVALID_PARAME= TER in OVMF Ia32
> with XCODE5 and CLANGPDB at least.
>
>= Fix this by allocating extra space in mNewGdt and then aligning the pointe= r
> upwards.

But I'm happy with this one.

Best Regards,

Leif

> Best wishe= s,
> Vitaly
>
> > On 5 Nov 2021, at 22:28, Leif Lindho= lm <leif@nuviainc.com> wrote= :
> >
> > UefiCpuPkg maintainers - please respond.
>= ; >
> > Meanwhile, Vitaly, could you please provide a commit me= ssage?
> > The BZ link is needed, but it's not a substitute.> >
> > /
> > =C2=A0 =C2=A0Leif
> >
&= gt; > On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote:
= > >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3639
>= >>
> >>
> >>
> >> Cc: Jiewen Yao= <jiewen.yao@intel.com>> >>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >>
> >>= ; Cc: Michael Kinney <mich= ael.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: Mar= vin H=C3=A4user <mhaeuser@posteo.d= e>
> >>
> >> Signed-off-by: Vitaly Cheptsov = <cheptsov@ispras.ru>
>= ; >>
> >> ---
> >>
> >> .../Libra= ry/CpuExceptionHandlerLib/DxeException.c =C2=A0 =C2=A0| 12 +++++++-----
= > >>
> >> 1 file changed, 7 insertions(+), 5 deletions= (-)
> >>
> >>
> >>
> >> dif= f --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c b/UefiCp= uPkg/Library/CpuExceptionHandlerLib/DxeException.c
> >>
>= >> index fd59f09ecd..12874811e1 100644
> >>
> >= > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
>= >>
> >> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/= DxeException.c
> >>
> >> @@ -22,7 +22,7 @@ EXCEPTIO= N_HANDLER_DATA =C2=A0 =C2=A0 =C2=A0mExceptionHandlerData;
> >><= br>> >>
> >>
> >> UINT8 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mNewStack[CPU_S= TACK_SWITCH_EXCEPTION_NUMBER *
> >>
> >> =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CPU_KNOWN_GOOD_STACK_SIZE]= ;
> >>
> >> -UINT8 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mNewGdt[CPU_TSS_GDT_SIZE];> >>
> >> +UINT8 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mNewGdt[CPU_TSS_GDT_SIZE + IA32_G= DT_ALIGNMENT];
> >>
> >>
> >>
> &= gt;> /**
> >>
> >> =C2=A0 Common exception handl= er.
> >>
> >> @@ -238,6 +238,7 @@ InitializeCpuExce= ptionHandlersEx (
> >>
> >> =C2=A0 CPU_EXCEPTION_IN= IT_DATA =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 EssData;
> >>
>= ; >> =C2=A0 IA32_DESCRIPTOR =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 Idtr;
> >>
> >> =C2=A0 IA32_D= ESCRIPTOR =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Gd= tr;
> >>
> >> + =C2=A0UINT8 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 *Gdt;
> >>
> >>
> >>
> >&g= t; =C2=A0 //
> >>
> >> =C2=A0 // To avoid repeat in= itialization of default handlers, the caller should pass
> >>> >> @@ -259,6 +260,7 @@ InitializeCpuExceptionHandlersEx (
&= gt; >>
> >> =C2=A0 =C2=A0 if (PcdGetBool (PcdCpuStackGuar= d)) {
> >>
> >> =C2=A0 =C2=A0 =C2=A0 if (InitData = =3D=3D NULL) {
> >>
> >> =C2=A0 =C2=A0 =C2=A0 =C2= =A0 SetMem (mNewGdt, sizeof (mNewGdt), 0);
> >>
> >>= ; + =C2=A0 =C2=A0 =C2=A0 =C2=A0Gdt =3D ALIGN_POINTER (mNewGdt, IA32_GDT_ALI= GNMENT);
> >>
> >>
> >>
> >>= ; =C2=A0 =C2=A0 =C2=A0 =C2=A0 AsmReadIdtr (&Idtr);
> >>
= > >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 AsmReadGdtr (&Gdtr);
> = >>
> >> @@ -270,11 +272,11 @@ InitializeCpuExceptionHandl= ersEx (
> >>
> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 EssDa= ta.X64.StackSwitchExceptionNumber =3D CPU_STACK_SWITCH_EXCEPTION_NUMBER;> >>
> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 EssData.X64.Idt= Table =3D (VOID *)Idtr.Base;
> >>
> >> =C2=A0 =C2= =A0 =C2=A0 =C2=A0 EssData.X64.IdtTableSize =3D Idtr.Limit + 1;
> >= >
> >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0EssData.X64.GdtTable =3D= mNewGdt;
> >>
> >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0Es= sData.X64.GdtTableSize =3D sizeof (mNewGdt);
> >>
> >&= gt; - =C2=A0 =C2=A0 =C2=A0 =C2=A0EssData.X64.ExceptionTssDesc =3D mNewGdt += Gdtr.Limit + 1;
> >>
> >> + =C2=A0 =C2=A0 =C2=A0 = =C2=A0EssData.X64.GdtTable =3D Gdt;
> >>
> >> + =C2= =A0 =C2=A0 =C2=A0 =C2=A0EssData.X64.GdtTableSize =3D CPU_TSS_GDT_SIZE;
&= gt; >>
> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0EssData.X64.Exce= ptionTssDesc =3D Gdt + Gdtr.Limit + 1;
> >>
> >> = =C2=A0 =C2=A0 =C2=A0 =C2=A0 EssData.X64.ExceptionTssDescSize =3D CPU_TSS_DE= SC_SIZE;
> >>
> >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0Ess= Data.X64.ExceptionTss =3D mNewGdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
= > >>
> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0EssData.X64.Exc= eptionTss =3D Gdt + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
> >>> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 EssData.X64.ExceptionTssSize =3D= CPU_TSS_SIZE;
> >>
> >>
> >>
> &= gt;> =C2=A0 =C2=A0 =C2=A0 =C2=A0 InitData =3D &EssData;
> >= >
> >> --
> >>
> >> 2.30.1 (Apple Gi= t-130)
> >>
> >>
> >>
> >><= br>> >>
> >>
> >>
> >>
<= /div> --000000000000ad3b8105d0477a35--