Message-ID: <287c2195-740c-4f2e-a545-c886962fc542@intel.com>
Date: Tue, 23 Sep 2025 22:12:53 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Sean Christopherson <seanjc@...gle.com>, Chao Gao <chao.gao@...el.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>,
 Binbin Wu <binbin.wu@...ux.intel.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/23/2025 4:04 AM, Sean Christopherson wrote:
> From: Sean Christopherson<seanjc@...gle.com>
> Date: Fri, 19 Sep 2025 15:32:25 -0700
> Subject: [PATCH] KVM: x86: Don't emulate instructions affected by CET features
> 
> 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 JMPs
> and CALLs, all of which are effectively indirect.  Explicitly check for
> emulation of IRET, FAR RET (IMM), and SYSEXIT (the ret-like far branches)
> instead of adding another flag, e.g. IsRet, as it's unlikely the emulator
> will ever need to check for return-like instructions outside of this one
> specific flow.  Use an allow-list instead of a deny-list because (a) it's
> a shorter list and (b) so that a missed entry gets a false positive, not a
> false negative (i.e. reject emulation instead of clobbering CET state).
> 
> 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>
> Reviewed-by: Chao Gao<chao.gao@...el.com>
> Reviewed-by: Binbin Wu<binbin.wu@...ux.intel.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@...el.com>

Two nits besides,
> Link:https://lore.kernel.org/r/20250919223258.1604852-19-seanjc@google.com
> Signed-off-by: Sean Christopherson<seanjc@...gle.com>
> ---
>   arch/x86/kvm/emulate.c | 117 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 103 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 23929151a5b8..a7683dc18405 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)
>   
> @@ -4068,9 +4069,9 @@ static const struct opcode group4[] = {
>   static const struct opcode group5[] = {
>   	F(DstMem | SrcNone | Lock,		em_inc),
>   	F(DstMem | SrcNone | Lock,		em_dec),
> -	I(SrcMem | NearBranch | IsBranch,       em_call_near_abs),
> -	I(SrcMemFAddr | ImplicitOps | IsBranch, em_call_far),
> -	I(SrcMem | NearBranch | IsBranch,       em_jmp_abs),
> +	I(SrcMem | NearBranch | IsBranch | ShadowStack, em_call_near_abs),
> +	I(SrcMemFAddr | ImplicitOps | IsBranch | ShadowStack, em_call_far),
> +	I(SrcMem | NearBranch | IsBranch, em_jmp_abs),

The change of this line is unexpected, since it only changes the 
indentation of 'em_jmp_abs'
>   static unsigned imm_size(struct x86_emulate_ctxt *ctxt)
>   {
>   	unsigned size;
> @@ -4943,6 +4998,40 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
>   
>   	ctxt->execute = opcode.u.execute;
>   
> +	/*
> +	 * Reject emulation if KVM might need to emulate shadow stack updates
> +	 * and/or indirect branch tracking enforcement, which the emulator
> +	 * doesn't support.
> +	 */
> +	if ((is_ibt_instruction(ctxt) || is_shstk_instruction(ctxt)) &&
> +	    ctxt->ops->get_cr(ctxt, 4) & X86_CR4_CET) {
> +		u64 u_cet = 0, s_cet = 0;
> +
> +		/*
> +		 * Check both User and Supervisor on far transfers as inter-
> +		 * privilege level transfers are impacted by CET at the target
> +		 * privilege level, and that is not known at this time.  The
> +		 * the expectation is that the guest will not require emulation

Dobule 'the'

> +		 * of any CET-affected instructions at any privilege level.
> +		 */
> +		if (!(ctxt->d & NearBranch))
> +			u_cet = s_cet = CET_SHSTK_EN | CET_ENDBR_EN;
> +		else if (ctxt->ops->cpl(ctxt) == 3)
> +			u_cet = CET_SHSTK_EN | CET_ENDBR_EN;
> +		else
> +			s_cet = CET_SHSTK_EN | CET_ENDBR_EN;
> +
> +		if ((u_cet && ctxt->ops->get_msr(ctxt, MSR_IA32_U_CET, &u_cet)) ||
> +		    (s_cet && ctxt->ops->get_msr(ctxt, MSR_IA32_S_CET, &s_cet)))
> +			return EMULATION_FAILED;
> +
> +		if ((u_cet | s_cet) & CET_SHSTK_EN && is_shstk_instruction(ctxt))
> +			return EMULATION_FAILED;
> +
> +		if ((u_cet | s_cet) & CET_ENDBR_EN && is_ibt_instruction(ctxt))
> +			return EMULATION_FAILED;
> +	}
> +
>   	if (unlikely(emulation_type & EMULTYPE_TRAP_UD) &&
>   	    likely(!(ctxt->d & EmulateOnUD)))
>   		return EMULATION_FAILED;
> 
> base-commit: 88539a6a25bc7a7ed96952775152e0c3331fdcaf

Powered by blists - more mailing lists