| 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
| ||
|
Date: Wed, 27 Apr 2022 18:04:54 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sasha Levin <sashal@...nel.org>, linux-kernel@...r.kernel.org,
stable@...r.kernel.org
Cc: Peter Gonda <pgonda@...gle.com>,
Sean Christopherson <seanjc@...gle.com>,
Marc Orr <marcorr@...gle.com>, kvm@...r.kernel.org,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, shuah@...nel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH MANUALSEL 5.17 6/7] KVM: SEV: Allow SEV intra-host
migration of VM with mirrors
On 4/27/22 17:54, Sasha Levin wrote:
> From: Peter Gonda <pgonda@...gle.com>
>
> [ Upstream commit b2125513dfc0dd0ec5a9605138a3c356592cfb73 ]
>
> For SEV-ES VMs with mirrors to be intra-host migrated they need to be
> able to migrate with the mirror. This is due to that fact that all VMSAs
> need to be added into the VM with LAUNCH_UPDATE_VMSA before
> lAUNCH_FINISH. Allowing migration with mirrors allows users of SEV-ES to
> keep the mirror VMs VMSAs during migration.
>
> Adds a list of mirror VMs for the original VM iterate through during its
> migration. During the iteration the owner pointers can be updated from
> the source to the destination. This fixes the ASID leaking issue which
> caused the blocking of migration of VMs with mirrors.
>
> Signed-off-by: Peter Gonda <pgonda@...gle.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Marc Orr <marcorr@...gle.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: kvm@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Message-Id: <20220211193634.3183388-1-pgonda@...gle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Sasha Levin <sashal@...nel.org>
Too scary,
NACK
Paolo
> ---
> arch/x86/kvm/svm/sev.c | 57 ++++++++++++-------
> arch/x86/kvm/svm/svm.h | 3 +-
> .../selftests/kvm/x86_64/sev_migrate_tests.c | 47 ++++++++++-----
> 3 files changed, 73 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index fef975852582..553d6dbf19a2 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -258,6 +258,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> goto e_free;
>
> INIT_LIST_HEAD(&sev->regions_list);
> + INIT_LIST_HEAD(&sev->mirror_vms);
>
> return 0;
>
> @@ -1623,9 +1624,12 @@ static void sev_unlock_vcpus_for_migration(struct kvm *kvm)
> }
> }
>
> -static void sev_migrate_from(struct kvm_sev_info *dst,
> - struct kvm_sev_info *src)
> +static void sev_migrate_from(struct kvm *dst_kvm, struct kvm *src_kvm)
> {
> + struct kvm_sev_info *dst = &to_kvm_svm(dst_kvm)->sev_info;
> + struct kvm_sev_info *src = &to_kvm_svm(src_kvm)->sev_info;
> + struct kvm_sev_info *mirror;
> +
> dst->active = true;
> dst->asid = src->asid;
> dst->handle = src->handle;
> @@ -1639,6 +1643,30 @@ static void sev_migrate_from(struct kvm_sev_info *dst,
> src->enc_context_owner = NULL;
>
> list_cut_before(&dst->regions_list, &src->regions_list, &src->regions_list);
> +
> + /*
> + * If this VM has mirrors, "transfer" each mirror's refcount of the
> + * source to the destination (this KVM). The caller holds a reference
> + * to the source, so there's no danger of use-after-free.
> + */
> + list_cut_before(&dst->mirror_vms, &src->mirror_vms, &src->mirror_vms);
> + list_for_each_entry(mirror, &dst->mirror_vms, mirror_entry) {
> + kvm_get_kvm(dst_kvm);
> + kvm_put_kvm(src_kvm);
> + mirror->enc_context_owner = dst_kvm;
> + }
> +
> + /*
> + * If this VM is a mirror, remove the old mirror from the owners list
> + * and add the new mirror to the list.
> + */
> + if (is_mirroring_enc_context(dst_kvm)) {
> + struct kvm_sev_info *owner_sev_info =
> + &to_kvm_svm(dst->enc_context_owner)->sev_info;
> +
> + list_del(&src->mirror_entry);
> + list_add_tail(&dst->mirror_entry, &owner_sev_info->mirror_vms);
> + }
> }
>
> static int sev_es_migrate_from(struct kvm *dst, struct kvm *src)
> @@ -1708,15 +1736,6 @@ 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) {
> @@ -1738,7 +1757,8 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
> if (ret)
> goto out_source_vcpu;
> }
> - sev_migrate_from(dst_sev, src_sev);
> +
> + sev_migrate_from(kvm, source_kvm);
> kvm_vm_dead(source_kvm);
> cg_cleanup_sev = src_sev;
> ret = 0;
> @@ -2008,10 +2028,10 @@ 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++;
> + mirror_sev = &to_kvm_svm(kvm)->sev_info;
> + list_add_tail(&mirror_sev->mirror_entry, &source_sev->mirror_vms);
>
> /* Set enc_context_owner and copy its encryption context over */
> - mirror_sev = &to_kvm_svm(kvm)->sev_info;
> mirror_sev->enc_context_owner = source_kvm;
> mirror_sev->active = true;
> mirror_sev->asid = source_sev->asid;
> @@ -2019,6 +2039,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
> mirror_sev->es_active = source_sev->es_active;
> mirror_sev->handle = source_sev->handle;
> INIT_LIST_HEAD(&mirror_sev->regions_list);
> + INIT_LIST_HEAD(&mirror_sev->mirror_vms);
> ret = 0;
>
> /*
> @@ -2041,19 +2062,17 @@ 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 (!sev_guest(kvm))
> return;
>
> + WARN_ON(!list_empty(&sev->mirror_vms));
> +
> /* If this is a mirror_kvm release the enc_context_owner and skip sev cleanup */
> if (is_mirroring_enc_context(kvm)) {
> struct kvm *owner_kvm = sev->enc_context_owner;
> - struct kvm_sev_info *owner_sev = &to_kvm_svm(owner_kvm)->sev_info;
>
> mutex_lock(&owner_kvm->lock);
> - if (!WARN_ON(!owner_sev->num_mirrored_vms))
> - owner_sev->num_mirrored_vms--;
> + list_del(&sev->mirror_entry);
> mutex_unlock(&owner_kvm->lock);
> kvm_put_kvm(owner_kvm);
> return;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 86bcfed6599e..831bca1fdc29 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -81,7 +81,8 @@ struct kvm_sev_info {
> struct list_head regions_list; /* List of registered regions */
> u64 ap_jump_table; /* SEV-ES AP Jump Table address */
> struct kvm *enc_context_owner; /* Owner of copied encryption context */
> - unsigned long num_mirrored_vms; /* Number of VMs sharing this ASID */
> + struct list_head mirror_vms; /* List of VMs mirroring */
> + struct list_head mirror_entry; /* Use as a list entry of mirrors */
> struct misc_cg *misc_cg; /* For misc cgroup accounting */
> atomic_t migration_in_progress;
> };
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> index 80056bbbb003..2e5a42cb470b 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -341,35 +341,54 @@ static void test_sev_mirror_parameters(void)
>
> static void test_sev_move_copy(void)
> {
> - struct kvm_vm *dst_vm, *sev_vm, *mirror_vm, *dst_mirror_vm;
> - int ret;
> + struct kvm_vm *dst_vm, *dst2_vm, *dst3_vm, *sev_vm, *mirror_vm,
> + *dst_mirror_vm, *dst2_mirror_vm, *dst3_mirror_vm;
>
> sev_vm = sev_vm_create(/* es= */ false);
> dst_vm = aux_vm_create(true);
> + dst2_vm = aux_vm_create(true);
> + dst3_vm = aux_vm_create(true);
> mirror_vm = aux_vm_create(false);
> dst_mirror_vm = aux_vm_create(false);
> + dst2_mirror_vm = aux_vm_create(false);
> + dst3_mirror_vm = aux_vm_create(false);
>
> sev_mirror_create(mirror_vm->fd, sev_vm->fd);
> - ret = __sev_migrate_from(dst_vm->fd, sev_vm->fd);
> - TEST_ASSERT(ret == -1 && errno == EBUSY,
> - "Cannot migrate VM that has mirrors. ret %d, errno: %d\n", ret,
> - errno);
>
> - /* The mirror itself can be migrated. */
> sev_migrate_from(dst_mirror_vm->fd, mirror_vm->fd);
> - ret = __sev_migrate_from(dst_vm->fd, sev_vm->fd);
> - TEST_ASSERT(ret == -1 && errno == EBUSY,
> - "Cannot migrate VM that has mirrors. ret %d, errno: %d\n", ret,
> - errno);
> + sev_migrate_from(dst_vm->fd, sev_vm->fd);
> +
> + sev_migrate_from(dst2_vm->fd, dst_vm->fd);
> + sev_migrate_from(dst2_mirror_vm->fd, dst_mirror_vm->fd);
> +
> + sev_migrate_from(dst3_mirror_vm->fd, dst2_mirror_vm->fd);
> + sev_migrate_from(dst3_vm->fd, dst2_vm->fd);
> +
> + kvm_vm_free(dst_vm);
> + kvm_vm_free(sev_vm);
> + kvm_vm_free(dst2_vm);
> + kvm_vm_free(dst3_vm);
> + kvm_vm_free(mirror_vm);
> + kvm_vm_free(dst_mirror_vm);
> + kvm_vm_free(dst2_mirror_vm);
> + kvm_vm_free(dst3_mirror_vm);
>
> /*
> - * mirror_vm is not a mirror anymore, dst_mirror_vm is. Thus,
> - * the owner can be copied as soon as dst_mirror_vm is gone.
> + * Run similar test be destroy mirrors before mirrored VMs to ensure
> + * destruction is done safely.
> */
> - kvm_vm_free(dst_mirror_vm);
> + sev_vm = sev_vm_create(/* es= */ false);
> + dst_vm = aux_vm_create(true);
> + mirror_vm = aux_vm_create(false);
> + dst_mirror_vm = aux_vm_create(false);
> +
> + sev_mirror_create(mirror_vm->fd, sev_vm->fd);
> +
> + sev_migrate_from(dst_mirror_vm->fd, mirror_vm->fd);
> sev_migrate_from(dst_vm->fd, sev_vm->fd);
>
> kvm_vm_free(mirror_vm);
> + kvm_vm_free(dst_mirror_vm);
> kvm_vm_free(dst_vm);
> kvm_vm_free(sev_vm);
> }
Powered by blists - more mailing lists