Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CP Exception when CET enable #4867

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Fix CP Exception when CET enable #4867

merged 1 commit into from
Nov 10, 2023

Conversation

jiaxinwu
Copy link
Member

No description provided.

@jiaxinwu jiaxinwu changed the title Optimize semaphore sync between BSP and AP Fix CP Exception when CET enable Nov 3, 2023
@jiaxinwu jiaxinwu force-pushed the master branch 5 times, most recently from 8030caa to 2146586 Compare November 8, 2023 01:10
Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return
address (tianocore#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to tianocore#1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return
address (tianocore#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to tianocore#2.
#CP exception happens because the actual return address (tianocore#2)
doesn't match the return address stored in shadow stack (tianocore#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.

Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Eric Dong <eric.dong@intel.com>
@niruiyu niruiyu added the push Auto push patch series in PR if all checks pass label Nov 10, 2023
@mergify mergify bot merged commit 589f2e4 into tianocore:master Nov 10, 2023
123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants