Hi Liming & Mike, Could you help approve & merge this patch into stable tag? It has got below reviewed-by:
Reviewed-by: Laszlo Ersek <[email protected]> Reviewed-by: Ray Ni <[email protected]> Reviewed-by: Eric Dong <[email protected]> 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 <[email protected]>; [email protected]; Gao, Liming > <[email protected]>; Kinney, Michael D > <[email protected]> > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; Zeng, Star > <[email protected]>; Gerd Hoffmann <[email protected]>; Kumar, Rahul R > <[email protected]> > 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 <[email protected]> > > Sent: Wednesday, November 8, 2023 2:57 AM > > To: [email protected]; Wu, Jiaxin <[email protected]> > > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; Zeng, Star > > <[email protected]>; Gerd Hoffmann <[email protected]>; Kumar, Rahul > R > > <[email protected]> > > 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 <[email protected]> > > > Cc: Ray Ni <[email protected]> > > > Cc: Zeng Star <[email protected]> > > > Cc: Gerd Hoffmann <[email protected]> > > > Cc: Rahul Kumar <[email protected]> > > > Cc: Laszlo Ersek <[email protected]> > > > Signed-off-by: Jiaxin Wu <[email protected]> > > > --- > > > 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 <[email protected]> -=-=-=-=-=-=-=-=-=-=-=- 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/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
