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 <ler...@redhat.com> > Sent: Wednesday, November 8, 2023 2:57 AM > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star > <star.z...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Kumar, Rahul R > <rahul.r.ku...@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.d...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Zeng Star <star.z...@intel.com> > > Cc: Gerd Hoffmann <kra...@redhat.com> > > Cc: Rahul Kumar <rahul1.ku...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Jiaxin Wu <jiaxin...@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 <ler...@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110880): https://edk2.groups.io/g/devel/message/110880 Mute This Topic: https://groups.io/mt/102434876/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
