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

[Atish Patra <atish.patra@linux.dev>]

On 6/1/26 4:17 PM, sashiko-bot@kernel.org wrote:
> 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?

It looks possible for SEV VMs looking at the code but I don't the know 
code path
or the mirroring/migration use case well enough to judge how likely this 
scenario
in real world.

>>   		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?
NA. They don't have to be same vm type as per my understanding from the 
selftest code.
>>   		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?

As per my understanding, that is intentional as well.
Is it common to mirror SEV VMs with SVM_SEV_FEAT_DEBUG_SWAP enabled ?

@tom: Please let me know if any of the above statements are not true.