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 (#110875): https://edk2.groups.io/g/devel/message/110875 Mute This Topic: https://groups.io/mt/102434876/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
