Re: [PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `kvm->arch.vm_type` validation in `sev_vm_copy_enc_context_from` allows creating structurally inconsistent mirror VMs.
- [High] Missing `vmsa_features` copy in `sev_vm_copy_enc_context_from` breaks SEV-ES mirror VMs that use features like `DEBUG_SWAP`.
- [High] Concurrent list modification of `owner_sev_info->mirror_vms` without holding the owner VM's lock during mirror VM migration.
--

commit 5adb8a893221b0569310f07fd9fc7abca8088651
Author: Atish Patra <atishp@meta.com>

KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

This commit rejects SNP source VMs in migration/mirroring until proper SNP
state transfer is implemented.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  		return ret;
>  
>  	if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
> -	    sev_guest(kvm) || !sev_guest(source_kvm)) {
> +	    sev_guest(kvm) || !sev_guest(source_kvm) ||
> +	    sev_snp_guest(source_kvm)) {

[Severity: High]
This is a pre-existing issue, but I noticed a potential locking race when
an existing mirror VM is migrated via sev_vm_move_enc_context_from().

Locks are only acquired for the source mirror (source_kvm) and destination
VM (kvm):

arch/x86/kvm/svm/sev.c:sev_vm_move_enc_context_from() {
    ...
    ret = sev_lock_two_vms(kvm, source_kvm);
    if (ret)
        return ret;
    ...
}

Later, in sev_migrate_from(), it updates its owner's list of mirrors:

arch/x86/kvm/svm/sev.c:sev_migrate_from() {
    ...
    if (is_mirroring_enc_context(dst_kvm)) {
        struct kvm_sev_info *owner_sev_info =
            to_kvm_sev_info(dst->enc_context_owner);

        list_del(&src->mirror_entry);
        list_add_tail(&dst->mirror_entry, &owner_sev_info->mirror_vms);
    }
    ...
}

Since the required lock for enc_context_owner is omitted entirely, could
this concurrently modify the owner_sev_info->mirror_vms linked list if
userspace creates a new mirror via sev_vm_copy_enc_context_from(), which
locks the owner VM? Would this result in kernel list corruption?

>  		ret = -EINVAL;
>  		goto out_unlock;
>  	}
> @@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  	 * created after SEV/SEV-ES initialization, e.g. to init intercepts.
>  	 */
>  	if (sev_guest(kvm) || !sev_guest(source_kvm) ||
> +	    sev_snp_guest(source_kvm) ||
>  	    is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {

[Severity: High]
This isn't a bug introduced by this patch, but this block seems to be
missing the kvm->arch.vm_type != source_kvm->arch.vm_type check that
exists in sev_vm_move_enc_context_from().

Does this omission allow creating structurally inconsistent mirror VMs?
Userspace could potentially create a destination VM as KVM_X86_SNP_VM and
mirror a source VM of type KVM_X86_SEV_VM into it. Since KVM's core MMU
would treat the new VM as an SNP VM (due to arch.vm_type), but the SEV
code treats it as a legacy SEV VM (because ____sev_snp_guest evaluates
to false), could this produce an inconsistent internal state and allow
attaching guest_memfd to standard SEV VMs?

>  		ret = -EINVAL;
>  		goto e_unlock;

[Severity: High]
This is a pre-existing issue, but I noticed another detail in
sev_vm_copy_enc_context_from() just below this diff.

The initialization block for the mirror VM omits copying vmsa_features:

arch/x86/kvm/svm/sev.c:sev_vm_copy_enc_context_from() {
    ...
    mirror_sev->enc_context_owner = source_kvm;
    mirror_sev->active = true;
    mirror_sev->asid = source_sev->asid;
    mirror_sev->fd = source_sev->fd;
    mirror_sev->es_active = source_sev->es_active;
    mirror_sev->need_init = false;
    mirror_sev->handle = source_sev->handle;
    ...
}

This omits mirror_sev->vmsa_features = source_sev->vmsa_features.

Since SEV-ES VMs use vmsa_features to track capabilities like
SVM_SEV_FEAT_DEBUG_SWAP, would a mirrored SEV-ES VM incorrectly
initialize its vmsa_features to 0? When vCPUs are instantiated,
sev_es_init_vmcb() checks sev_vcpu_has_debug_swap(), which would evaluate
to false, resulting in misconfigured VMCB intercepts. Could this silently
drop important execution features for SEV-ES mirrors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-sev_snp_fixes-v2-0-611891b28a86@meta.com?part=1