Patches 1 and 2 are the long-awaited tests for COPY_ENC_CONTEXT_FROM,
based on the ones for intra-host migration. The aim of patches 3
and 4 is to simplify the locking for COPY_ENC_CONTEXT_FROM, and solving
(by sidestepping the question) the problem of a VM's encryption
context being moved from and copied from at the same time.
These patches are an alternative to Sean's patch with subject "KVM:
SEV: Explicitly document that there are no TOCTOU races in copy ASID"
(https://lore.kernel.org/kvm/[email protected]/T/).
There is another bug: a VM that is the owner of a copied context must not
be migrated, otherwise you could have 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
I'll take a look at this later, probably next week after this series has
been reviewed.
Paolo
Paolo Bonzini (4):
selftests: sev_migrate_tests: free all VMs
selftests: sev_migrate_tests: add tests for
KVM_CAP_VM_COPY_ENC_CONTEXT_FROM
KVM: SEV: cleanup locking for KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked
arch/x86/kvm/svm/sev.c | 118 ++++++++----------
.../selftests/kvm/x86_64/sev_migrate_tests.c | 113 +++++++++++++++--
2 files changed, 155 insertions(+), 76 deletions(-)
--
2.27.0
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 <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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;
+
/*
* 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
On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <[email protected]> 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 <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
Acked-by: Peter Gonda <[email protected]>
> ---
> 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
>
On 11/17/21 18:46, Peter Gonda wrote:
>> + 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.
>
It was covered by the locking test (which does not check i != j).
There's no equivalent for the operation of creating a mirror VM, I can
add it in v2.
Paolo