linux-kernel - Re: [PATCH 10/12] KVM: SEV: Prohibit migration of a VM that has mirrors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc583908-ee45-a320-2326-bfae926f0623@redhat.com>
Date:   Wed, 1 Dec 2021 19:21:56 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Peter Gonda <pgonda@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH 10/12] KVM: SEV: Prohibit migration of a VM that has
 mirrors

On 12/1/21 19:17, Peter Gonda wrote:
> On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <pbonzini@...hat.com> wrote:
>>
>> VMs that mirror an encryption context rely on the owner to keep the
>> ASID allocated.  Performing a KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
>> would cause a dangling ASID:
>>
>> 1. copy context from A to B (gets ref to A)
>> 2. move context from A to L (moves ASID from A to L)
>> 3. close L (releases ASID from L, B still references it)
>>
>> The right way to do the handoff instead is to create a fresh mirror VM
>> on the destination first:
>>
>> 1. copy context from A to B (gets ref to A)
>> [later] 2. close B (releases ref to A)
>> 3. move context from A to L (moves ASID from A to L)
>> 4. copy context from L to M
>>
>> So, catch the situation by adding a count of how many VMs are
>> mirroring this one's encryption context.
>>
>> Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
>> ---
>>   arch/x86/kvm/svm/sev.c                        | 22 ++++++++++-
>>   arch/x86/kvm/svm/svm.h                        |  1 +
>>   .../selftests/kvm/x86_64/sev_migrate_tests.c  | 37 +++++++++++++++++++
>>   3 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 025d9731b66c..89a716290fac 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -1696,6 +1696,16 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
>>          }
>>
>>          src_sev = &to_kvm_svm(source_kvm)->sev_info;
>> +
>> +       /*
>> +        * VMs mirroring src's encryption context rely on it to keep the
>> +        * ASID allocated, but below we are clearing src_sev->asid.
>> +        */
>> +       if (src_sev->num_mirrored_vms) {
>> +               ret = -EBUSY;
>> +               goto out_unlock;
>> +       }
>> +
>>          dst_sev->misc_cg = get_current_misc_cg();
>>          cg_cleanup_sev = dst_sev;
>>          if (dst_sev->misc_cg != src_sev->misc_cg) {
>> @@ -1987,6 +1997,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>>           */
>>          source_sev = &to_kvm_svm(source_kvm)->sev_info;
>>          kvm_get_kvm(source_kvm);
>> +       source_sev->num_mirrored_vms++;
>>
>>          /* Set enc_context_owner and copy its encryption context over */
>>          mirror_sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -2019,12 +2030,21 @@ void sev_vm_destroy(struct kvm *kvm)
>>          struct list_head *head = &sev->regions_list;
>>          struct list_head *pos, *q;
>>
>> +       WARN_ON(sev->num_mirrored_vms);
>> +
> 
> If we don't change to atomic doesn't this need to happen when we have
> the kvm->lock?

Hi,

there can be no concurrency when destroying a VM.  (If you do Rust, it's 
similar to using x.get_mut().get_mut() on an Arc<Mutex<T>>).

Paolo