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

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

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