| 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, 17 Nov 2021 10:46:48 -0700
From: Peter Gonda <pgonda@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
seanjc@...gle.com
Subject: Re: [PATCH 4/4] KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked
On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <pbonzini@...hat.com> wrote:
>
> Now that we have a facility to lock two VMs with deadlock
> protection, use it for the creation of mirror VMs as well. One of
> COPY_ENC_CONTEXT_FROM(dst, src) and COPY_ENC_CONTEXT_FROM(src, dst)
> would always fail, so the combination is nonsensical and it is okay to
> return -EBUSY if it is attempted.
>
> This sidesteps the question of what happens if a VM is
> MOVE_ENC_CONTEXT_FROM'd at the same time as it is
> COPY_ENC_CONTEXT_FROM'd: the locking prevents that from
> happening.
>
> Cc: Peter Gonda <pgonda@...gle.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
Acked-by: Peter Gonda <pgonda@...gle.com>
> ---
> arch/x86/kvm/svm/sev.c | 68 ++++++++++++++++--------------------------
> 1 file changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f9256ba269e6..47d54df7675c 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1548,6 +1548,9 @@ static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
> struct kvm_sev_info *dst_sev = &to_kvm_svm(dst_kvm)->sev_info;
> struct kvm_sev_info *src_sev = &to_kvm_svm(src_kvm)->sev_info;
>
> + if (dst_kvm == src_kvm)
> + return -EINVAL;
> +
Worth adding a migrate/mirror from self fails tests in
test_sev_(migrate|mirror)_parameters()? I guess it's already covered
by "the source cannot be SEV enabled" test cases.
> /*
> * Bail if these VMs are already involved in a migration to avoid
> * deadlock between two VMs trying to migrate to/from each other.
> @@ -1951,76 +1954,57 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
> {
> struct file *source_kvm_file;
> struct kvm *source_kvm;
> - struct kvm_sev_info source_sev, *mirror_sev;
> + struct kvm_sev_info *source_sev, *mirror_sev;
> int ret;
>
> source_kvm_file = fget(source_fd);
> if (!file_is_kvm(source_kvm_file)) {
> ret = -EBADF;
> - goto e_source_put;
> + goto e_source_fput;
> }
>
> source_kvm = source_kvm_file->private_data;
> - mutex_lock(&source_kvm->lock);
> -
> - if (!sev_guest(source_kvm)) {
> - ret = -EINVAL;
> - goto e_source_unlock;
> - }
> + ret = sev_lock_two_vms(kvm, source_kvm);
> + if (ret)
> + goto e_source_fput;
>
> - /* Mirrors of mirrors should work, but let's not get silly */
> - if (is_mirroring_enc_context(source_kvm) || source_kvm == kvm) {
> + /*
> + * Mirrors of mirrors should work, but let's not get silly. Also
> + * disallow out-of-band SEV/SEV-ES init if the target is already an
> + * SEV guest, or if vCPUs have been created. KVM relies on vCPUs being
> + * created after SEV/SEV-ES initialization, e.g. to init intercepts.
> + */
> + if (sev_guest(kvm) || !sev_guest(source_kvm) ||
> + is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
> ret = -EINVAL;
> - goto e_source_unlock;
> + goto e_unlock;
> }
>
> - memcpy(&source_sev, &to_kvm_svm(source_kvm)->sev_info,
> - sizeof(source_sev));
> -
> /*
> * The mirror kvm holds an enc_context_owner ref so its asid can't
> * disappear until we're done with it
> */
> + source_sev = &to_kvm_svm(source_kvm)->sev_info;
> kvm_get_kvm(source_kvm);
>
> - fput(source_kvm_file);
> - mutex_unlock(&source_kvm->lock);
> - mutex_lock(&kvm->lock);
> -
> - /*
> - * Disallow out-of-band SEV/SEV-ES init if the target is already an
> - * SEV guest, or if vCPUs have been created. KVM relies on vCPUs being
> - * created after SEV/SEV-ES initialization, e.g. to init intercepts.
> - */
> - if (sev_guest(kvm) || kvm->created_vcpus) {
> - ret = -EINVAL;
> - goto e_mirror_unlock;
> - }
> -
> /* 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;
> - mirror_sev->fd = source_sev.fd;
> - mirror_sev->es_active = source_sev.es_active;
> - mirror_sev->handle = source_sev.handle;
> + mirror_sev->asid = source_sev->asid;
> + mirror_sev->fd = source_sev->fd;
> + mirror_sev->es_active = source_sev->es_active;
> + mirror_sev->handle = source_sev->handle;
> + ret = 0;
> /*
> * Do not copy ap_jump_table. Since the mirror does not share the same
> * KVM contexts as the original, and they may have different
> * memory-views.
> */
>
> - mutex_unlock(&kvm->lock);
> - return 0;
> -
> -e_mirror_unlock:
> - mutex_unlock(&kvm->lock);
> - kvm_put_kvm(source_kvm);
> - return ret;
> -e_source_unlock:
> - mutex_unlock(&source_kvm->lock);
> -e_source_put:
> +e_unlock:
> + sev_unlock_two_vms(kvm, source_kvm);
> +e_source_fput:
> if (source_kvm_file)
> fput(source_kvm_file);
> return ret;
> --
> 2.27.0
>
Powered by blists - more mailing lists