Re: [PATCH v3 4/8] KVM: SEV: Add direct VMSA capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: SEV: snp_launch_update_vmsa() skips debug and state checks on direct VMSA
- [Low] KVM: SVM: unscoped guard(mutex) in switch case in svm_enable_vm_cap()
--

commit ab904a5d7e5b93c050ee3612b960971e0dec53a9
Author: Joerg Roedel <joerg.roedel@amd.com>

KVM: SEV: Add direct VMSA capability

This commit adds the VM-scoped KVM_CAP_SNP_DIRECT_VMSA capability so
userspace can explicitly opt into direct VMSA mode on an SNP VM before
creating vCPUs. When enabled, KVM skips allocating KVM-owned VMSAs during
vCPU creation and skips syncing and measuring KVM-owned VMSAs at launch
finish.

[ ... ]

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5bfe23790ba46..5638786c1d03d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2541,8 +2541,15 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		struct vcpu_svm *svm = to_svm(vcpu);
> -		u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
> +		u64 pfn;
> +
> +		if (sev->snp_direct_vmsa) {
> +			if (!svm->sev_es.snp_has_guest_vmsa)
> +				svm->vmcb->control.vmsa_pa = INVALID_PAGE;
> +			goto protect_vcpu;
> +		}
>  
> +		pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
>  		ret = sev_es_sync_vmsa(svm);
>  		if (ret)
>  			goto out;

[Severity: Medium]
When sev->snp_direct_vmsa is set in snp_launch_update_vmsa(), jumping
directly to protect_vcpu skips sev_es_sync_vmsa(), which normally performs
these pre-launch checks:

arch/x86/kvm/svm/sev.c:sev_es_sync_vmsa() {
	if (vcpu->arch.guest_state_protected)
		return -EINVAL;

	/* Check some debug related fields before encrypting the VMSA */
	if (svm->vcpu.guest_debug || (svm->vmcb->save.dr7 & ~DR7_FIXED_1))
		return -EINVAL;
}

Because kvm_arch_vcpu_ioctl_set_guest_debug() and
kvm_vcpu_ioctl_x86_set_debugregs() only check
vcpu->arch.guest_state_protected, userspace can invoke KVM_SET_GUEST_DEBUG
or KVM_SET_DEBUGREGS on an SNP vCPU before calling KVM_SEV_SNP_LAUNCH_FINISH.

Could skipping these checks allow an SNP guest with KVM_CAP_SNP_DIRECT_VMSA
enabled to finalize launch with unsupported host guest_debug state (such as
BP_VECTOR intercepts, single-step, hardware breakpoint switching via
KVM_DEBUGREG_BP_ENABLED, or APICV_INHIBIT_REASON_BLOCKIRQ) locked in once
vcpu->arch.guest_state_protected is set, or allow snp_launch_update_vmsa() to
run again on an already-protected vCPU?

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 136069dfcd44f..d841093941156 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5458,6 +5458,27 @@ static void *svm_alloc_apic_backing_page(struct kvm_vcpu *vcpu)
>  	return page_address(page);
>  }
>  
> +static int svm_enable_vm_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
> +{
> +	switch (cap->cap) {
> +#ifdef CONFIG_KVM_AMD_SEV
> +	case KVM_CAP_SNP_DIRECT_VMSA:
> +		if (memchr_inv(cap->args, 0, sizeof(cap->args)) ||
> +		    kvm->arch.vm_type != KVM_X86_SNP_VM)
> +			return -EINVAL;
> +
> +		guard(mutex)(&kvm->lock);
> +		if (kvm->created_vcpus)
> +			return -EINVAL;
> +
> +		to_kvm_sev_info(kvm)->snp_direct_vmsa = true;
> +		return 0;
> +#endif
> +	default:
> +		return -EINVAL;
> +	}
> +}

[Severity: Low]
As reported by Sashiko on v2 [1], in svm_enable_vm_cap(), guard(mutex)
declares a cleanup-attributed variable inside the switch block without
enclosing braces around the KVM_CAP_SNP_DIRECT_VMSA case.

When cap->cap does not match KVM_CAP_SNP_DIRECT_VMSA, jumping to the default
label bypasses initialization of the guard variable while remaining in its
scope, causing a build failure with CONFIG_KVM_AMD_SEV enabled. Should the
case body be wrapped in braces or use scoped_guard()?

[1] https://lore.kernel.org/r/20260908103338.427254-5-joro@8bytes.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102114137.1198982-1-joro@8bytes.org?part=4