Message-ID: <fd8ebddd-adfc-4eef-bf30-20139574d0dd@linux.intel.com>
Date: Mon, 22 Sep 2025 13:39:50 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, Tom Lendacky <thomas.lendacky@....com>,
 Mathias Krause <minipli@...ecurity.net>, John Allen <john.allen@....com>,
 Rick Edgecombe <rick.p.edgecombe@...el.com>, Chao Gao <chao.gao@...el.com>,
 Xiaoyao Li <xiaoyao.li@...el.com>, Maxim Levitsky <mlevitsk@...hat.com>,
 Zhang Yi Z <yi.z.zhang@...ux.intel.com>, Xin Li <xin@...or.com>
Subject: Re: [PATCH v16 18/51] KVM: x86: Don't emulate instructions affected
 by CET features



On 9/20/2025 6:32 AM, 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@...el.com>
> Originally-by: Yang Weijiang <weijiang.yang@...el.com>
> Cc: Mathias Krause <minipli@...ecurity.net>
> Cc: John Allen <john.allen@....com>
> Cc: Rick Edgecombe <rick.p.edgecombe@...el.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>

Reviewed-by: Binbin Wu <binbin.wu@...ux.intel.com>

Two nits below.

> ---
>   arch/x86/kvm/emulate.c | 114 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 100 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 23929151a5b8..dc0249929cbf 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -178,6 +178,7 @@
>   #define IncSP       ((u64)1 << 54)  /* SP is incremented before ModRM calc */
>   #define TwoMemOp    ((u64)1 << 55)  /* Instruction has two memory operand */
>   #define IsBranch    ((u64)1 << 56)  /* Instruction is considered a branch. */
> +#define ShadowStack ((u64)1 << 57)  /* Instruction affects Shadow Stacks. */
>   
>   #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
>   
> @@ -660,6 +661,57 @@ static inline bool emul_is_noncanonical_address(u64 la,
>   	return !ctxt->ops->is_canonical_addr(ctxt, la, flags);
>   }
>   
> +static bool is_shstk_instruction(u64 flags)
> +{
> +	return flags & ShadowStack;
> +}
> +
> +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.

SYCALL -> SYSCALL

> +	 */
> +	if (!(flags & NearBranch))
> +		return true;
> +
> +	switch (flags & (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.
> +	 */
> +	case SrcImmU16:
> +		return false;
> +	default:
> +		WARN_ONCE(1, "Unexpected Src operand '%llx' on branch",
> +			  (flags & (OpMask << SrcShift)));
> +		return false;

Is it safer to reject the emulation if it has unexpected src operand?

> +	}
> +}
> +
>
[...]

Powered by blists - more mailing lists