public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM
@ 2025-09-08 20:17 John Allen
  2025-09-08 20:17 ` [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs John Allen
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: John Allen @ 2025-09-08 20:17 UTC (permalink / raw)
  To: kvm, linux-kernel, x86, seanjc, pbonzini, dave.hansen
  Cc: rick.p.edgecombe, mlevitsk, weijiang.yang, chao.gao, bp,
	dave.hansen, hpa, mingo, tglx, thomas.lendacky, John Allen

AMD Zen3 and newer processors support shadow stack, a feature designed
to protect against ROP (return-oriented programming) attacks in which an
attacker manipulates return addresses on the call stack in order to
execute arbitrary code. To prevent this, shadow stacks can be allocated
that are only used by control transfer and return instructions. When a
CALL instruction is issued, it writes the return address to both the
program stack and the shadow stack. When the subsequent RET instruction
is issued, it pops the return address from both stacks and compares
them. If the addresses don't match, a control-protection exception is
raised.

Shadow stack and a related feature, Indirect Branch Tracking (IBT), are
collectively referred to as Control-flow Enforcement Technology (CET).
However, current AMD processors only support shadow stack and not IBT.

This series adds support for shadow stack in SVM guests and builds upon
the support added in the CET guest support patch series [1]. Additional
patches are required to support shadow stack enabled guests in qemu [2].

[1]: CET guest support patches (v13)
https://lore.kernel.org/all/20250821133132.72322-1-chao.gao@intel.com/

[2]: CET qemu patches
https://lore.kernel.org/all/20230720111445.99509-1-weijiang.yang@intel.com/

[3]:  Previous SVM support patches (v3)
https://lore.kernel.org/all/20250806204510.59083-1-john.allen@amd.com/

---

RFC v2:
  - Rebased on v3 of the Intel CET virtualization series, dropping the
    patch that moved cet_is_msr_accessible to common code as that has
    been pulled into the Intel series.
  - Minor change removing curly brackets around if statement introduced
    in patch 6/6.
RFC v3:
  - Rebased on v5 of the Intel CET virtualization series.
  - Add patch changing the name of vmplX_ssp SEV-ES save area fields to
    plX_ssp.
  - Merge this series intended for KVM with the separate guest kernel
    patch (now patch 7/8).
  - Update MSR passthrough code to conditionally pass through shadow
    stack MSRS based on both host and guest support.
  - Don't save PL0_SSP, PL1_SSP, and PL2_SSP MSRs on SEV-ES VMRUN as
    these are currently unused.
v1:
  - Remove RFC tag from series
  - Rebase on v6 of the Intel CET virtualization series
  - Use KVM-governed feature to track SHSTK for SVM
v2:
  - Add new patch renaming boot_*msr to raw_*msr. Utilize raw_rdmsr when
    reading XSS on SEV-ES cpuid instructions.
  - Omit unnecessary patch for saving shadow stack msrs on SEV-ES VMRUN
  - Omit passing through of XSS for SEV-ES as support has already been
    properly implemented in a26b7cd22546 ("KVM: SEV: Do not intercept
    accesses to MSR_IA32_XSS for SEV-ES guests") 
v3:
  - Rebased on v11 of the Intel CET Virtualization series.
  - Split guest kernel patches into a separate series as these are
    independent of this series and are needed to support non-KVM
    hypervisors.
v4:
  - Rebased on v13 of the Intel CET Virtualization series.
  - Add SEV-ES save area fields to dump_vmcb.
  - Don't pass through MSR_IA32_INT_SSP_TAB.
  - Don't remove clearing of IBT capability.

John Allen (5):
  KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs
  KVM: x86: SVM: Update dump_vmcb with shadow stack save area additions
  KVM: x86: SVM: Pass through shadow stack MSRs
  KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
  KVM: SVM: Enable shadow stack virtualization for SVM

 arch/x86/kvm/svm/sev.c |  5 +++++
 arch/x86/kvm/svm/svm.c | 43 +++++++++++++++++++++++++++++++++++++++---
 arch/x86/kvm/svm/svm.h |  1 +
 3 files changed, 46 insertions(+), 3 deletions(-)

-- 
2.47.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs
  2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
@ 2025-09-08 20:17 ` John Allen
  2025-09-12 22:55   ` Sean Christopherson
  2025-09-08 20:17 ` [PATCH v4 2/5] KVM: x86: SVM: Update dump_vmcb with shadow stack save area additions John Allen
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: John Allen @ 2025-09-08 20:17 UTC (permalink / raw)
  To: kvm, linux-kernel, x86, seanjc, pbonzini, dave.hansen
  Cc: rick.p.edgecombe, mlevitsk, weijiang.yang, chao.gao, bp,
	dave.hansen, hpa, mingo, tglx, thomas.lendacky, John Allen

Emulate shadow stack MSR access by reading and writing to the
corresponding fields in the VMCB.

Signed-off-by: John Allen <john.allen@amd.com>
---
 arch/x86/kvm/svm/svm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e4af4907c7d8..fee60f3378e1 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2767,6 +2767,15 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (guest_cpuid_is_intel_compatible(vcpu))
 			msr_info->data |= (u64)svm->sysenter_esp_hi << 32;
 		break;
+	case MSR_IA32_S_CET:
+		msr_info->data = svm->vmcb->save.s_cet;
+		break;
+	case MSR_IA32_INT_SSP_TAB:
+		msr_info->data = svm->vmcb->save.isst_addr;
+		break;
+	case MSR_KVM_INTERNAL_GUEST_SSP:
+		msr_info->data = svm->vmcb->save.ssp;
+		break;
 	case MSR_TSC_AUX:
 		msr_info->data = svm->tsc_aux;
 		break;
@@ -2999,6 +3008,15 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		svm->vmcb01.ptr->save.sysenter_esp = (u32)data;
 		svm->sysenter_esp_hi = guest_cpuid_is_intel_compatible(vcpu) ? (data >> 32) : 0;
 		break;
+	case MSR_IA32_S_CET:
+		svm->vmcb->save.s_cet = data;
+		break;
+	case MSR_IA32_INT_SSP_TAB:
+		svm->vmcb->save.isst_addr = data;
+		break;
+	case MSR_KVM_INTERNAL_GUEST_SSP:
+		svm->vmcb->save.ssp = data;
+		break;
 	case MSR_TSC_AUX:
 		/*
 		 * TSC_AUX is always virtualized for SEV-ES guests when the
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 2/5] KVM: x86: SVM: Update dump_vmcb with shadow stack save area additions
  2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
  2025-09-08 20:17 ` [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs John Allen
@ 2025-09-08 20:17 ` John Allen
  2025-09-08 20:17 ` [PATCH v4 3/5] KVM: x86: SVM: Pass through shadow stack MSRs John Allen
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: John Allen @ 2025-09-08 20:17 UTC (permalink / raw)
  To: kvm, linux-kernel, x86, seanjc, pbonzini, dave.hansen
  Cc: rick.p.edgecombe, mlevitsk, weijiang.yang, chao.gao, bp,
	dave.hansen, hpa, mingo, tglx, thomas.lendacky, John Allen

Add shadow stack VMCB fields to dump_vmcb. PL0_SSP, PL1_SSP, PL2_SSP,
PL3_SSP, and U_CET are part of the SEV-ES save area and are encrypted,
but can be decrypted and dumped if the guest policy allows debugging.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: John Allen <john.allen@amd.com>
---
v4:
  - Dump shstk fields in sev-es save area.
---
 arch/x86/kvm/svm/svm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index fee60f3378e1..aee1bb8c01d0 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3407,6 +3407,10 @@ static void dump_vmcb(struct kvm_vcpu *vcpu)
 	       "rip:", save->rip, "rflags:", save->rflags);
 	pr_err("%-15s %016llx %-13s %016llx\n",
 	       "rsp:", save->rsp, "rax:", save->rax);
+	pr_err("%-15s %016llx %-13s %016llx\n",
+	       "s_cet:", save->s_cet, "ssp:", save->ssp);
+	pr_err("%-15s %016llx\n",
+	       "isst_addr:", save->isst_addr);
 	pr_err("%-15s %016llx %-13s %016llx\n",
 	       "star:", save01->star, "lstar:", save01->lstar);
 	pr_err("%-15s %016llx %-13s %016llx\n",
@@ -3431,6 +3435,13 @@ static void dump_vmcb(struct kvm_vcpu *vcpu)
 		pr_err("%-15s %016llx\n",
 		       "sev_features", vmsa->sev_features);
 
+		pr_err("%-15s %016llx %-13s %016llx\n",
+		       "pl0_ssp:", vmsa->pl0_ssp, "pl1_ssp:", vmsa->pl1_ssp);
+		pr_err("%-15s %016llx %-13s %016llx\n",
+		       "pl2_ssp:", vmsa->pl2_ssp, "pl3_ssp:", vmsa->pl3_ssp);
+		pr_err("%-15s %016llx\n",
+		       "u_cet:", vmsa->u_cet);
+
 		pr_err("%-15s %016llx %-13s %016llx\n",
 		       "rax:", vmsa->rax, "rbx:", vmsa->rbx);
 		pr_err("%-15s %016llx %-13s %016llx\n",
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 3/5] KVM: x86: SVM: Pass through shadow stack MSRs
  2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
  2025-09-08 20:17 ` [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs John Allen
  2025-09-08 20:17 ` [PATCH v4 2/5] KVM: x86: SVM: Update dump_vmcb with shadow stack save area additions John Allen
@ 2025-09-08 20:17 ` John Allen
  2025-09-08 20:17 ` [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel John Allen
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: John Allen @ 2025-09-08 20:17 UTC (permalink / raw)
  To: kvm, linux-kernel, x86, seanjc, pbonzini, dave.hansen
  Cc: rick.p.edgecombe, mlevitsk, weijiang.yang, chao.gao, bp,
	dave.hansen, hpa, mingo, tglx, thomas.lendacky, John Allen

Pass through XSAVE managed CET MSRs on SVM when KVM supports shadow
stack. These cannot be intercepted without also intercepting XSAVE which
would likely cause unacceptable performance overhead.
MSR_IA32_INT_SSP_TAB is not managed by XSAVE, so it is intercepted.

Reviewed-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: John Allen <john.allen@amd.com>
---
v4:
  - Don't pass through MSR_IA32_INT_SSP_TAB
---
 arch/x86/kvm/svm/svm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index aee1bb8c01d0..b18573b530aa 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -844,6 +844,17 @@ static void svm_recalc_msr_intercepts(struct kvm_vcpu *vcpu)
 		svm_disable_intercept_for_msr(vcpu, MSR_IA32_MPERF, MSR_TYPE_R);
 	}
 
+	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK)) {
+		bool shstk_enabled = guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK);
+
+		svm_set_intercept_for_msr(vcpu, MSR_IA32_U_CET, MSR_TYPE_RW, !shstk_enabled);
+		svm_set_intercept_for_msr(vcpu, MSR_IA32_S_CET, MSR_TYPE_RW, !shstk_enabled);
+		svm_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP, MSR_TYPE_RW, !shstk_enabled);
+		svm_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP, MSR_TYPE_RW, !shstk_enabled);
+		svm_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP, MSR_TYPE_RW, !shstk_enabled);
+		svm_set_intercept_for_msr(vcpu, MSR_IA32_PL3_SSP, MSR_TYPE_RW, !shstk_enabled);
+	}
+
 	if (sev_es_guest(vcpu->kvm))
 		sev_es_recalc_msr_intercepts(vcpu);
 
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
  2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
                   ` (2 preceding siblings ...)
  2025-09-08 20:17 ` [PATCH v4 3/5] KVM: x86: SVM: Pass through shadow stack MSRs John Allen
@ 2025-09-08 20:17 ` John Allen
  2025-09-10 21:24   ` Sean Christopherson
  2025-09-08 20:17 ` [PATCH v4 5/5] KVM: SVM: Enable shadow stack virtualization for SVM John Allen
  2025-09-12 22:54 ` [PATCH v4 0/5] Enable Shadow Stack Virtualization " Sean Christopherson
  5 siblings, 1 reply; 12+ messages in thread
From: John Allen @ 2025-09-08 20:17 UTC (permalink / raw)
  To: kvm, linux-kernel, x86, seanjc, pbonzini, dave.hansen
  Cc: rick.p.edgecombe, mlevitsk, weijiang.yang, chao.gao, bp,
	dave.hansen, hpa, mingo, tglx, thomas.lendacky, John Allen

When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
be intercepting the CPUID instruction and will need to access the guest
MSR_IA32_XSS value. For SEV-ES, the XSS value is encrypted and needs to be
included in the GHCB to be visible to the hypervisor.

Signed-off-by: John Allen <john.allen@amd.com>
---
v2:
  - Omit passing through XSS as this has already been properly
    implemented in a26b7cd22546 ("KVM: SEV: Do not intercept
    accesses to MSR_IA32_XSS for SEV-ES guests")
v3:
  - Move guest kernel GHCB_ACCESSORS definition to new series.
v4:
  - Change logic structure to be more intuitive.
---
 arch/x86/kvm/svm/sev.c | 5 +++++
 arch/x86/kvm/svm/svm.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f4381878a9e5..33c42dd853b3 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3310,6 +3310,11 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
 		vcpu->arch.cpuid_dynamic_bits_dirty = true;
 	}
 
+	if (kvm_ghcb_xss_is_valid(svm)) {
+		vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
+		vcpu->arch.cpuid_dynamic_bits_dirty = true;
+	}
+
 	/* Copy the GHCB exit information into the VMCB fields */
 	exit_code = ghcb_get_sw_exit_code(ghcb);
 	control->exit_code = lower_32_bits(exit_code);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 3c7f208b7935..552c58b050f1 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -935,5 +935,6 @@ DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_1)
 DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_2)
 DEFINE_KVM_GHCB_ACCESSORS(sw_scratch)
 DEFINE_KVM_GHCB_ACCESSORS(xcr0)
+DEFINE_KVM_GHCB_ACCESSORS(xss)
 
 #endif
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 5/5] KVM: SVM: Enable shadow stack virtualization for SVM
  2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
                   ` (3 preceding siblings ...)
  2025-09-08 20:17 ` [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel John Allen
@ 2025-09-08 20:17 ` John Allen
  2025-09-12 22:54 ` [PATCH v4 0/5] Enable Shadow Stack Virtualization " Sean Christopherson
  5 siblings, 0 replies; 12+ messages in thread
From: John Allen @ 2025-09-08 20:17 UTC (permalink / raw)
  To: kvm, linux-kernel, x86, seanjc, pbonzini, dave.hansen
  Cc: rick.p.edgecombe, mlevitsk, weijiang.yang, chao.gao, bp,
	dave.hansen, hpa, mingo, tglx, thomas.lendacky, John Allen

Remove the explicit clearing of shadow stack CPU capabilities.

Reviewed-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: John Allen <john.allen@amd.com>
---
v4:
  - Don't remove clearing of IBT feature.
---
 arch/x86/kvm/svm/svm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b18573b530aa..304531d6c8b0 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5262,10 +5262,7 @@ static __init void svm_set_cpu_caps(void)
 	kvm_set_cpu_caps();
 
 	kvm_caps.supported_perf_cap = 0;
-	kvm_caps.supported_xss = 0;
 
-	/* KVM doesn't yet support CET virtualization for SVM. */
-	kvm_cpu_cap_clear(X86_FEATURE_SHSTK);
 	kvm_cpu_cap_clear(X86_FEATURE_IBT);
 
 	/* CPUID 0x80000001 and 0x8000000A (SVM features) */
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
  2025-09-08 20:17 ` [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel John Allen
@ 2025-09-10 21:24   ` Sean Christopherson
  2025-09-11 15:23     ` John Allen
  0 siblings, 1 reply; 12+ messages in thread
From: Sean Christopherson @ 2025-09-10 21:24 UTC (permalink / raw)
  To: John Allen
  Cc: kvm, linux-kernel, x86, pbonzini, dave.hansen, rick.p.edgecombe,
	mlevitsk, weijiang.yang, chao.gao, bp, dave.hansen, hpa, mingo,
	tglx, thomas.lendacky

On Mon, Sep 08, 2025, John Allen wrote:
> When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
> be intercepting the CPUID instruction and will need to access the guest
> MSR_IA32_XSS value. For SEV-ES, the XSS value is encrypted and needs to be
> included in the GHCB to be visible to the hypervisor.
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---
> v2:
>   - Omit passing through XSS as this has already been properly
>     implemented in a26b7cd22546 ("KVM: SEV: Do not intercept
>     accesses to MSR_IA32_XSS for SEV-ES guests")
> v3:
>   - Move guest kernel GHCB_ACCESSORS definition to new series.

Except that broke _this_ series.

arch/x86/kvm/svm/sev.c: In function ‘sev_es_sync_from_ghcb’:
arch/x86/kvm/svm/sev.c:3293:39: error: implicit declaration of function ‘ghcb_get_xss’;
                                       did you mean ‘ghcb_get_rsi’? [-Wimplicit-function-declaration]
 3293 |                 vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
      |                                       ^~~~~~~~~~~~
      |                                       ghcb_get_rsi
  AR      drivers/base/built-in.a
  AR      drivers/built-in.a

> v4:
>   - Change logic structure to be more intuitive.
> ---
>  arch/x86/kvm/svm/sev.c | 5 +++++
>  arch/x86/kvm/svm/svm.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f4381878a9e5..33c42dd853b3 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3310,6 +3310,11 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>  		vcpu->arch.cpuid_dynamic_bits_dirty = true;
>  	}
>  
> +	if (kvm_ghcb_xss_is_valid(svm)) {
> +		vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);

Honestly, I think the ghcb_get_xxx() helpers do more harm than good.  For set()
and if_valid(), I'm totally on board with a wrapper.  For get(), unless we WARN
on trying to read an invalid field, I just don't see the point.  Ugh, and we
_can't_ WARN, at least not in KVM, because of the whole TOCTOU mess.

Case in point, this and the xcr0 check can elide setting cpuid_dynamic_bits_dirty
if XCR0/XSS isn't actually changing, but then this

	if (kvm_ghcb_xcr0_is_valid(svm) && vcpu->arch.xcr0 != ghcb_get_xcr0(ghcb)) {
		vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
		vcpu->arch.cpuid_dynamic_bits_dirty = true;
	}

looks wonky unless the reader knows that ghcb_get_xcr0() is just reading a struct
field, which obviously isn't terribly difficult to figure out, but the macros
make it more than a bit annoying.

Argh, even worse, that check is technically subject to a TOCTOU bug as well.  It
just doesn't matter in practice because the guest can only hose it self, e.g. by
swizzling XCR0/XSS.  But it's still flawed.

And for both XCR0/XSS, KVM lets the guest throw garbage into vcpu->arch.xcr0 and
now vcpu->arch.xss.  Maybe that's not problematic in practice, but I'd rather not
find out the hard way.

Lastly, open coding the write to cpuid_dynamic_bits_dirty and vcpu->arch.xcr0 is
just gross.

So to avoid a rather pointless dependency for CET, which I'm trying my darndest
to land in 6.18, I'm going to put together a separate fixup patch and replace
this patchh to end up with code that does:

	if (kvm_ghcb_xcr0_is_valid(svm)
		__kvm_set_xcr(vcpu, 0, kvm_ghcb_get_xcr0(ghcb));

	if (kvm_ghcb_xss_is_valid(svm))
		__kvm_emulate_msr_write(vcpu, MSR_IA32_XSS, kvm_ghcb_get_xss(ghcb));

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
  2025-09-10 21:24   ` Sean Christopherson
@ 2025-09-11 15:23     ` John Allen
  0 siblings, 0 replies; 12+ messages in thread
From: John Allen @ 2025-09-11 15:23 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, linux-kernel, x86, pbonzini, dave.hansen, rick.p.edgecombe,
	mlevitsk, weijiang.yang, chao.gao, bp, dave.hansen, hpa, mingo,
	tglx, thomas.lendacky

On Wed, Sep 10, 2025 at 02:24:29PM -0700, Sean Christopherson wrote:
> On Mon, Sep 08, 2025, John Allen wrote:
> > When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
> > be intercepting the CPUID instruction and will need to access the guest
> > MSR_IA32_XSS value. For SEV-ES, the XSS value is encrypted and needs to be
> > included in the GHCB to be visible to the hypervisor.
> > 
> > Signed-off-by: John Allen <john.allen@amd.com>
> > ---
> > v2:
> >   - Omit passing through XSS as this has already been properly
> >     implemented in a26b7cd22546 ("KVM: SEV: Do not intercept
> >     accesses to MSR_IA32_XSS for SEV-ES guests")
> > v3:
> >   - Move guest kernel GHCB_ACCESSORS definition to new series.
> 
> Except that broke _this_ series.
> 
> arch/x86/kvm/svm/sev.c: In function ‘sev_es_sync_from_ghcb’:
> arch/x86/kvm/svm/sev.c:3293:39: error: implicit declaration of function ‘ghcb_get_xss’;
>                                        did you mean ‘ghcb_get_rsi’? [-Wimplicit-function-declaration]
>  3293 |                 vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
>       |                                       ^~~~~~~~~~~~
>       |                                       ghcb_get_rsi
>   AR      drivers/base/built-in.a
>   AR      drivers/built-in.a

Apologies, that series should be considered a prerequisite for this
series. I pulled the guest kernel patch into a separate series since it
doesn't depend on the main series and we ideally would want it to be
pulled in ASAP rather than wait on the rest of the series since it
enables linux guests running on non-KVM hypervisors.

Thanks,
John

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM
  2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
                   ` (4 preceding siblings ...)
  2025-09-08 20:17 ` [PATCH v4 5/5] KVM: SVM: Enable shadow stack virtualization for SVM John Allen
@ 2025-09-12 22:54 ` Sean Christopherson
  2025-09-15 14:52   ` John Allen
  5 siblings, 1 reply; 12+ messages in thread
From: Sean Christopherson @ 2025-09-12 22:54 UTC (permalink / raw)
  To: John Allen
  Cc: kvm, linux-kernel, x86, pbonzini, dave.hansen, rick.p.edgecombe,
	mlevitsk, weijiang.yang, chao.gao, bp, dave.hansen, hpa, mingo,
	tglx, thomas.lendacky

On Mon, Sep 08, 2025, John Allen wrote:
> This series adds support for shadow stack in SVM guests
                  ^
                  |
                some

I mean, who cares about nested, right?

Sorry for being snippy, but I am more than a bit peeved that we're effectively
on revision 6 of this series, and apparently no one has thought to do even basic
tested of nested SVM.  And I'm even more grumpy that writing tests continues to
be low priority in general, which is especially concerning for such a large,
complex feature.

Adding support for nested was easy enough (famous last words), but I really wish
I could get back the ~hour I spent figuring out what was missing...

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs
  2025-09-08 20:17 ` [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs John Allen
@ 2025-09-12 22:55   ` Sean Christopherson
  0 siblings, 0 replies; 12+ messages in thread
From: Sean Christopherson @ 2025-09-12 22:55 UTC (permalink / raw)
  To: John Allen
  Cc: kvm, linux-kernel, x86, pbonzini, dave.hansen, rick.p.edgecombe,
	mlevitsk, weijiang.yang, chao.gao, bp, dave.hansen, hpa, mingo,
	tglx, thomas.lendacky

On Mon, Sep 08, 2025, John Allen wrote:
> Emulate shadow stack MSR access by reading and writing to the
> corresponding fields in the VMCB.
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---
>  arch/x86/kvm/svm/svm.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e4af4907c7d8..fee60f3378e1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2767,6 +2767,15 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		if (guest_cpuid_is_intel_compatible(vcpu))
>  			msr_info->data |= (u64)svm->sysenter_esp_hi << 32;
>  		break;
> +	case MSR_IA32_S_CET:
> +		msr_info->data = svm->vmcb->save.s_cet;
> +		break;
> +	case MSR_IA32_INT_SSP_TAB:
> +		msr_info->data = svm->vmcb->save.isst_addr;
> +		break;
> +	case MSR_KVM_INTERNAL_GUEST_SSP:
> +		msr_info->data = svm->vmcb->save.ssp;
> +		break;
>  	case MSR_TSC_AUX:
>  		msr_info->data = svm->tsc_aux;
>  		break;
> @@ -2999,6 +3008,15 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		svm->vmcb01.ptr->save.sysenter_esp = (u32)data;
>  		svm->sysenter_esp_hi = guest_cpuid_is_intel_compatible(vcpu) ? (data >> 32) : 0;
>  		break;
> +	case MSR_IA32_S_CET:
> +		svm->vmcb->save.s_cet = data;

These writes should mark VMCB_CET (the dirty/clean flag) dirty, and obviously
KVM should mark VMCB_CET clean along with everything else on #VMEXIT.

> +		break;
> +	case MSR_IA32_INT_SSP_TAB:
> +		svm->vmcb->save.isst_addr = data;
> +		break;
> +	case MSR_KVM_INTERNAL_GUEST_SSP:
> +		svm->vmcb->save.ssp = data;
> +		break;
>  	case MSR_TSC_AUX:
>  		/*
>  		 * TSC_AUX is always virtualized for SEV-ES guests when the
> -- 
> 2.47.3
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM
  2025-09-12 22:54 ` [PATCH v4 0/5] Enable Shadow Stack Virtualization " Sean Christopherson
@ 2025-09-15 14:52   ` John Allen
  2025-09-15 16:46     ` Sean Christopherson
  0 siblings, 1 reply; 12+ messages in thread
From: John Allen @ 2025-09-15 14:52 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, linux-kernel, x86, pbonzini, dave.hansen, rick.p.edgecombe,
	mlevitsk, weijiang.yang, chao.gao, bp, dave.hansen, hpa, mingo,
	tglx, thomas.lendacky

On Fri, Sep 12, 2025 at 03:54:31PM -0700, Sean Christopherson wrote:
> On Mon, Sep 08, 2025, John Allen wrote:
> > This series adds support for shadow stack in SVM guests
>                   ^
>                   |
>                 some
> 
> I mean, who cares about nested, right?
> 
> Sorry for being snippy, but I am more than a bit peeved that we're effectively
> on revision 6 of this series, and apparently no one has thought to do even basic
> tested of nested SVM.

Hi Sean,

I have been testing nested with this feature (or so I thought). Can you
explain what you did to test and what wasn't working?

Apologies, and thanks for taking the time to look into the problem.

Thanks,
John

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM
  2025-09-15 14:52   ` John Allen
@ 2025-09-15 16:46     ` Sean Christopherson
  0 siblings, 0 replies; 12+ messages in thread
From: Sean Christopherson @ 2025-09-15 16:46 UTC (permalink / raw)
  To: John Allen
  Cc: kvm, linux-kernel, x86, pbonzini, dave.hansen, rick.p.edgecombe,
	mlevitsk, weijiang.yang, chao.gao, bp, dave.hansen, hpa, mingo,
	tglx, thomas.lendacky

On Mon, Sep 15, 2025, John Allen wrote:
> On Fri, Sep 12, 2025 at 03:54:31PM -0700, Sean Christopherson wrote:
> > On Mon, Sep 08, 2025, John Allen wrote:
> > > This series adds support for shadow stack in SVM guests
> >                   ^
> >                   |
> >                 some
> > 
> > I mean, who cares about nested, right?
> > 
> > Sorry for being snippy, but I am more than a bit peeved that we're effectively
> > on revision 6 of this series, and apparently no one has thought to do even basic
> > tested of nested SVM.
> 
> Hi Sean,
> 
> I have been testing nested with this feature (or so I thought).

The issue here is that Linux only supports shadow stacks at CPL3, i.e. only
exercises MSR_IA32_U_CET, and for whatever reason the KVM-Unit-Test only tests
MSR_IA32_U_CET too (and is stupidly not compatible with AMD due to requiring
SHSTK *and* IBT).  So just running those in nested won't provide any coverage
for S_CET.

> Can you explain what you did to test and what wasn't working?

Read/write MSR_IA32_S_CET to a non-zero value from L2 by running the proposed
selftest[*] in L1.  Because KVM doesn't propagate S_CET to/from vmcb12, the
writes from L2 are effectively lost.

An ever better way to cover this would be a selftest or KUT test to explicitly
read/write MSRs in L2, and/or fill vmcs12/vmcb12 from L1 and verify L2 sees the
desired value.

https://lore.kernel.org/all/20250912232319.429659-37-seanjc@google.com

> Apologies, and thanks for taking the time to look into the problem.

No worries, I didn't intend to single you out, I was essentially just yelling at
everyone involved :-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-09-15 16:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-08 20:17 [PATCH v4 0/5] Enable Shadow Stack Virtualization for SVM John Allen
2025-09-08 20:17 ` [PATCH v4 1/5] KVM: x86: SVM: Emulate reads and writes to shadow stack MSRs John Allen
2025-09-12 22:55   ` Sean Christopherson
2025-09-08 20:17 ` [PATCH v4 2/5] KVM: x86: SVM: Update dump_vmcb with shadow stack save area additions John Allen
2025-09-08 20:17 ` [PATCH v4 3/5] KVM: x86: SVM: Pass through shadow stack MSRs John Allen
2025-09-08 20:17 ` [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel John Allen
2025-09-10 21:24   ` Sean Christopherson
2025-09-11 15:23     ` John Allen
2025-09-08 20:17 ` [PATCH v4 5/5] KVM: SVM: Enable shadow stack virtualization for SVM John Allen
2025-09-12 22:54 ` [PATCH v4 0/5] Enable Shadow Stack Virtualization " Sean Christopherson
2025-09-15 14:52   ` John Allen
2025-09-15 16:46     ` Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox