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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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 (#111002): https://edk2.groups.io/g/devel/message/111002
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]
-=-=-=-=-=-=-=-=-=-=-=-