Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7712 bytes --]

merged.

Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Friday, November 10, 2023 8:43 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Gao, Liming <gaoliming@byosoft.com.cn>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable

Thank you, Mike, the PR (https://github.com/tianocore/edk2/pull/4867) has been synced & updated with reviewed by tag, and we can merge once pass the CI check.



> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, November 10, 2023 8:25 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> I approve this change for edk2-stable202311
>
> The PR looks out of sync with this email patch.
>
> Can you please update PR with latest patch and commit
> message that was reviewed and add review tags?
>
> Mike
>
> > -----Original Message-----
> > From: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Sent: Thursday, November 9, 2023 4:01 PM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> > Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> CP
> > Exception when CET enable
> >
> > Hi Liming & Mike,
> >
> > Could you help approve & merge this patch into stable tag? It has got
> > below reviewed-by:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > Reviewed-by: Eric Dong <eric.dong@intel.com>
> >
> > I also created the PR: https://github.com/tianocore/edk2/pull/4867
> >
> > Thanks,
> > Jiaxin
> >
> >
> > > -----Original Message-----
> > > From: Wu, Jiaxin
> > > Sent: Wednesday, November 8, 2023 9:17 AM
> > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> > Liming
> > > <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star
> > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> > Rahul R
> > > <rahul.r.kumar@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> > CP
> > > Exception when CET enable
> > >
> > > Hi Liming & Mike & Ray,
> > >
> > > Could you help approve this change for the coming edk2 stable tag?
> > This is
> > > critical bug fix in smm cpu driver to handler the CET check failure,
> > I think we
> > > need this change for the stable tag.
> > >
> > > Thanks,
> > > Jiaxin
> > >
> > > > -----Original Message-----
> > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > Sent: Wednesday, November 8, 2023 2:57 AM
> > > > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> > > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star
> > > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> > Rahul
> > > R
> > > > <rahul.r.kumar@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm:
> > Fix
> > > CP
> > > > Exception when CET enable
> > > >
> > > > On 11/7/23 02:24, Wu, Jiaxin wrote:
> > > > > Root cause:
> > > > > 1. Before DisableReadonlyPageWriteProtect() is called, the
> > return
> > > > > address (#1) is pushed in shadow stack.
> > > > > 2. CET is disabled.
> > > > > 3. DisableReadonlyPageWriteProtect() returns to #1.
> > > > > 4. Page table is modified.
> > > > > 5. EnableReadonlyPageWriteProtect() is called, but the return
> > > > > address (#2) is not pushed in shadow stack.
> > > > > 6. CET is enabled.
> > > > > 7. EnableReadonlyPageWriteProtect() returns to #2.
> > > > > #CP exception happens because the actual return address (#2)
> > > > > doesn't match the return address stored in shadow stack (#1).
> > > > >
> > > > > Analysis:
> > > > > Shadow stack will stop update after CET disable (DisableCet() in
> > > > > DisableReadOnlyPageWriteProtect), but normal smi stack will be
> > > > > continue updated with the function called and return
> > > > > (DisableReadOnlyPageWriteProtect &
> > EnableReadOnlyPageWriteProtect),
> > > > > thus leading stack mismatch after CET re-enabled (EnableCet() in
> > > > > EnableReadOnlyPageWriteProtect).
> > > > >
> > > > > According SDM Vol 3, 6.15-Control Protection Exception:
> > > > > Normal smi stack and shadow stack must be matched when CET
> > enable,
> > > > > otherwise CP Exception will happen, which is caused by a near
> > RET
> > > > > instruction.
> > > > >
> > > > > CET is disabled in DisableCet(), while can be enabled in
> > > > > EnableCet(). This way won't cause the problem because they are
> > > > > implemented in a way that return address of DisableCet() is
> > > > > poped out from shadow stack (Incsspq performs a pop to increases
> > > > > the shadow stack) and EnableCet() doesn't use "RET" but "JMP" to
> > > > > return to caller. So calling EnableCet() and DisableCet()
> > doesn't
> > > > > have the same issue as calling DisableReadonlyPageWriteProtect()
> > > > > and EnableReadonlyPageWriteProtect().
> > > > >
> > > > > With above root cause & analysis, define below 2 macros instead
> > of
> > > > > functions for WP & CET operation:
> > > > > WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
> > > > > WRITE_PROTECT_RO_PAGES (Wp, Cet)
> > > > > Because DisableCet() & EnableCet() must be in the same function
> > > > > to avoid shadow stack and normal SMI stack mismatch.
> > > > >
> > > > > Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
> > > > > WRITE_PROTECT_RO_PAGES () in same function.
> > > > >
> > > > > Cc: Eric Dong <eric.dong@intel.com>
> > > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > > Cc: Zeng Star <star.zeng@intel.com>
> > > > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > > > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> > > > > ---
> > > > >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 59
> > > > +++++++++++++----
> > > > >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |
> 73
> > > > +++++++++-------------
> > > > >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c             |  7 ++-
> > > > >  3 files changed, 81 insertions(+), 58 deletions(-)
> > > >
> > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111022): https://edk2.groups.io/g/devel/message/111022
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 12288 bytes --]