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

["Laszlo Ersek" <lersek@redhat.com>]

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