Re: [PATCH 2/4] kvm: svm: Defer VMSA allocation to LAUNCH_FINISH stage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing VMSA cleanup on the error path in `snp_init_kvm_vmsa()` causes a guest-reachable `WARN_ON_ONCE` upon retry.
- [Medium] Late vCPU creation results in KVM's INVALID_PAGE sentinel being passed directly to the hardware PSP firmware when dumping VMCB.
--

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

kvm: svm: Defer VMSA allocation to LAUNCH_FINISH stage
    
Do not allocate a KVM-managed VMSA for all VCPUs on VCPU creation,
defer it to the LAUNCH_FINISH stage of SEV-ES and SEV-SNP. At this
stage the VMSAs get used for the first time.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 9e00aa7bdca8c..0aa62cb802fe5 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c

[ ... ]

> @@ -2689,12 +2700,14 @@ 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);
> -		void *vmsa = sev_es_vmsa_ref(vcpu);
> +		void *vmsa;
>  
>  		ret = sev_es_sync_vmsa(svm);
>  		if (ret)
>  			goto out;
>  
> +		vmsa = sev_es_vmsa_ref(vcpu);
> +
>  		ret = sev_es_vcpu_vmsa_make_private(vcpu);
>  		if (ret)
>  			goto out;

[Severity: Medium]
Does this error path leak the newly allocated VMSA?

Since sev_es_sync_vmsa() now allocates the VMSA, if
sev_es_vcpu_vmsa_make_private() fails, the code returns via the "out" label
without freeing the VMSA or resetting its state.

If userspace retries the KVM_SEV_SNP_LAUNCH_FINISH ioctl, sev_es_sync_vmsa()
is called again and will trigger the WARN_ON_ONCE() in
sev_es_vcpu_alloc_vmsa() because the state is still VMSA_SHARED:

    if (WARN_ON_ONCE(svm->sev_es.vmsa.vmsa_state != VMSA_NONE))
        return -EINVAL;

Can we add a call to sev_es_vcpu_free_vmsa() on this error path?

[ ... ]

> @@ -4889,22 +4906,11 @@ void sev_init_vmcb(struct vcpu_svm *svm, bool init_event)
>  int sev_vcpu_create(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> -	int ret;
>  
>  	mutex_init(&svm->sev_es.snp_vmsa_mutex);
>  
> -	if (!is_sev_es_guest(vcpu))
> -		return 0;
> -
> -	/*
> -	 * SEV-ES guests require a separate (from the VMCB) VMSA page used to
> -	 * contain the encrypted register state of the guest.
> -	 */
> -	ret = sev_es_vcpu_alloc_vmsa(vcpu);
> -	if (ret)
> -		return ret;
> -
> -	vcpu->arch.guest_tsc_protected = snp_is_secure_tsc_enabled(vcpu->kvm);
> +	if (is_sev_es_guest(vcpu))
> +		vcpu->arch.guest_tsc_protected = snp_is_secure_tsc_enabled(vcpu->kvm);
>  
>  	return 0;
>  }

[Severity: Medium]
Does removing this allocation lead to KVM's INVALID_PAGE sentinel being passed
to the PSP firmware?

If userspace creates a vCPU after the SEV-ES or SEV-SNP launch sequence has
completed, the new vCPU's VMSA is never allocated.

When this vCPU is run, sev_es_vmsa_pa() returns KVM's INVALID_PAGE sentinel
(~0ull), which is loaded into the VMCB, triggering an immediate #VMEXIT
(SVM_EXIT_ERR).

If the dump_invalid_vmcb parameter is enabled, KVM handles this by calling
dump_vmcb(), which then calls sev_decrypt_vmsa(). Since sev_es_vmsa_ref()
returns NULL for VMSA_NONE, sev_decrypt_vmsa() erroneously assumes the VMSA
is encrypted and passes the INVALID_PAGE sentinel directly to the PSP
firmware as the source physical address for SEV_CMD_DBG_DECRYPT:

    data.src_addr = svm->vmcb->control.vmsa_pa;
    data.len = PAGE_SIZE;

    ret = sev_issue_cmd(vcpu->kvm, SEV_CMD_DBG_DECRYPT, &data, &argp_err);

Could this direct hardware exposure to an invalid page address risk firmware
hangs or IOMMU faults?

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