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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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 (#111001): https://edk2.groups.io/g/devel/message/111001
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-