Re: [PATCH 1/2] KVM: SEV: Free have_run_cpus during VM destruction even if VM is no longer SEV

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kvm: double free of sev->have_run_cpus on initialization failure
--

commit 35fcc7370ece167eff16a7738a6c47e4815c1ae4
Author: Sean Christopherson <seanjc@google.com>

KVM: SEV: Free have_run_cpus during VM destruction even if VM is no longer SEV

Unconditionally free SEV's "have run CPUs" cpumask in the VM destroy path,
i.e. even for what appear to be non-SEV VMs, as an SEV VM becomes a non-SEV
VM if its state is intra-host migrated.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 63e8cfa9bf559..c9242c936a401 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2981,13 +2981,17 @@ void sev_vm_destroy(struct kvm *kvm)
>  	struct list_head *head = &sev->regions_list;
>  	struct list_head *pos, *q;
>  
> +	/*
> +	 * Free the mask even if the VM is not *currently* an SEV VM, as it may
> +	 * have been an SEV VM prior to intra-host migration.
> +	 */
> +	free_cpumask_var(sev->have_run_cpus);

[Severity: Critical]
Can this cause a double free of sev->have_run_cpus?

If a user issues a KVM_SEV_INIT ioctl and __sev_guest_init() fails after
allocating the cpumask, the error path in __sev_guest_init() frees the
mask but leaves the pointer intact:

arch/x86/kvm/svm/sev.c:__sev_guest_init() {
    ...
e_free:
	free_cpumask_var(sev->have_run_cpus);
    ...
}

Because free_cpumask_var() receives the mask by value, the pointer remains
dangling. When the user later closes the VM file descriptor,
kvm_destroy_vm() calls sev_vm_destroy().

Prior to this patch, the early return on !sev_guest(kvm) prevented the second
free. By moving free_cpumask_var() before the !sev_guest(kvm) check, won't
we unconditionally double free the dangling pointer?

> +
>  	if (!sev_guest(kvm))
>  		return;
>  
>  	WARN_ON(!list_empty(&sev->mirror_vms));
>  
> -	free_cpumask_var(sev->have_run_cpus);
> -
>  	/*
>  	 * If this is a mirror VM, remove it from the owner's list of a mirrors
>  	 * and skip ASID cleanup (the ASID is tied to the lifetime of the owner).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923163721.1584779-1-seanjc@google.com?part=1