Re: [PATCH v16 18/51] KVM: x86: Don't emulate instructions affected by CET features

Chao Gao
2025-09-22T10:28:13Z

On Fri, Sep 19, 2025 at 03:32:25PM -0700, Sean Christopherson wrote:
>Don't emulate branch instructions, e.g. CALL/RET/JMP etc., that are
>affected by Shadow Stacks and/or Indirect Branch Tracking when said
>features are enabled in the guest, as fully emulating CET would require
>significant complexity for no practical benefit (KVM shouldn't need to
>emulate branch instructions on modern hosts). Simply doing nothing isn't
>an option as that would allow a malicious entity to subvert CET
>protections via the emulator.
>
>To detect instructions that are subject to IBT or affect IBT state, use
>the existing IsBranch flag along with the source operand type to detect
>indirect branches, and the existing NearBranch flag to detect far branches
>(which can affect IBT state even if the branch itself is direct).
>
>For Shadow Stacks, explicitly track instructions that directly affect the
>current SSP, as KVM's emulator doesn't have existing flags that can be
>used to precisely detect such instructions. Alternatively, the em_xxx()
>helpers could directly check for ShadowStack interactions, but using a
>dedicated flag is arguably easier to audit, and allows for handling both
>IBT and SHSTK in one fell swoop.
>
>Note! On far transfers, do NOT consult the current privilege level and
>instead treat SHSTK/IBT as being enabled if they're enabled for User *or*
>Supervisor mode. On inter-privilege level far transfers, SHSTK and IBT
>can be in play for the target privilege level, i.e. checking the current
>privilege could get a false negative, and KVM doesn't know the target
>privilege level until emulation gets under way.
>
>Note #2, FAR JMP from 64-bit mode to compatibility mode interacts with
>the current SSP, but only to ensure SSP[63:32] == 0. Don't tag FAR JMP
>as SHSTK, which would be rather confusing and would result in FAR JMP
>being rejected unnecessarily the vast majority of the time (ignoring that
>it's unlikely to ever be emulated). A future commit will add the #GP(0)
>check for the specific FAR JMP scenario.
>
>Note #3, task switches also modify SSP and so need to be rejected. That
>too will be addressed in a future commit.
>
>Suggested-by: Chao Gao <chao.gao@intel.com>
>Originally-by: Yang Weijiang <weijiang.yang@intel.com>
>Cc: Mathias Krause <minipli@grsecurity.net>
>Cc: John Allen <john.allen@amd.com>
>Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
>Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

<snip>
>+static bool is_ibt_instruction(u64 flags)
>+{
>+ if (!(flags & IsBranch))
>+ return false;
>+
>+ /*
>+ * Far transfers can affect IBT state even if the branch itself is
>+ * direct, e.g. when changing privilege levels and loading a conforming
>+ * code segment. For simplicity, treat all far branches as affecting
>+ * IBT. False positives are acceptable (emulating far branches on an
>+ * IBT-capable CPU won't happen in practice), while false negatives
>+ * could impact guest security.
>+ *
>+ * Note, this also handles SYCALL and SYSENTER.
>+ */
>+ if (!(flags & NearBranch))
>+ return true;
>+
>+ switch (flags & (OpMask << SrcShift)) {

nit: maybe use SrcMask here.

#define SrcMask (OpMask << SrcShift)

>+ case SrcReg:
>+ case SrcMem:
>+ case SrcMem16:
>+ case SrcMem32:
>+ return true;
>+ case SrcMemFAddr:
>+ case SrcImmFAddr:
>+ /* Far branches should be handled above. */
>+ WARN_ON_ONCE(1);
>+ return true;
>+ case SrcNone:
>+ case SrcImm:
>+ case SrcImmByte:
>+ /*
>+ * Note, ImmU16 is used only for the stack adjustment operand on ENTER
>+ * and RET instructions. ENTER isn't a branch and RET FAR is handled
>+ * by the NearBranch check above. RET itself isn't an indirect branch.
>+ */

RET FAR isn't affected by IBT, right? So it is a false positive in the above
NearBranch check. I am not asking you to fix it - just want to ensure it is
intended.

>+ case SrcImmU16:
>+ return false;
>+ default:
>+ WARN_ONCE(1, "Unexpected Src operand '%llx' on branch",
>+ (flags & (OpMask << SrcShift)));

Ditto. use SrcMask here.