KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

PATCH Superseded Peter Gonda 4 participants 12 messages Sep 14, 2021 – Sep 21, 2021

Subsystems (4)

Stable branch x86 KVM KVM x86

lore

Share & tools

lore

Peter Gonda

author

2021-09-14 17:15 UTC #0 lore raw

PATCH 1/1 KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

Copying an ASID into new vCPUs will not work for SEV-ES since the vCPUs VMSAs need to be setup and measured before SEV_LAUNCH_FINISH. Return an error if a users tries to KVM_CAP_VM_COPY_ENC_CONTEXT_FROM from an SEV-ES guest.

Fixes 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")

Signed-off-by Peter Gonda <[email protected]>

Cc Marc Orr <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Sean Christopherson <[email protected]>

Cc Nathan Tempelman <[email protected]>

Cc Brijesh Singh <[email protected]>

Cc [email protected]

---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kvm/svm/sev.c +1 -1

index 75e0b21ad07c..8a279027425f 100644
@@ -1728,7 +1728,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
 	source_kvm = source_kvm_file->private_data;
 	mutex_lock(&source_kvm->lock);
 
-	if (!sev_guest(source_kvm)) {
+	if (!sev_guest(source_kvm) || sev_es_guest(source_kvm)) {
 		ret = -EINVAL;
 		goto e_source_unlock;
 	}

Signature · 1 line

2.33.0.309.g3052b89438-goog

Sean Christopherson

maintainer

2021-09-14 17:33 UTC #1 lore raw

Quoted 4 lines · Peter Gonda

Copying an ASID into new vCPUs will not work for SEV-ES since the vCPUs VMSAs need to be setup and measured before SEV_LAUNCH_FINISH. Return an error if a users tries to KVM_CAP_VM_COPY_ENC_CONTEXT_FROM from an SEV-ES guest.

What happens if userspace does KVM_CAP_VM_COPY_ENC_CONTEXT_FROM before the source has created vCPUs, i.e. before it has done SEV_LAUNCH_FINISH?

Might be worth noting that the destination cannot be an SEV guest, and therefore can't be an SEV-ES guest either.

Quoted 1 line

Fixes 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")

Cc [email protected]

Peter Gonda

author

2021-09-14 17:58 UTC #2 lore raw

Quoted 9 lines · Sean Christopherson

Quoted 4 lines · Peter Gonda
Copying an ASID into new vCPUs will not work for SEV-ES since the vCPUs VMSAs need to be setup and measured before SEV_LAUNCH_FINISH. Return an error if a users tries to KVM_CAP_VM_COPY_ENC_CONTEXT_FROM from an SEV-ES guest.
What happens if userspace does KVM_CAP_VM_COPY_ENC_CONTEXT_FROM before the source has created vCPUs, i.e. before it has done SEV_LAUNCH_FINISH?

That's not enough. If you wanted to be able to mirror SEV-ES you'd also need to call LAUNCH_UPDATE_VMSA on the mirror's vCPUs before SEV_LAUNCH_FINISH. That is do-able but I was writing a small change to fix this bug. If mirroring of SEV-ES is wanted it's a much bigger change.

Quoted 3 lines

Might be worth noting that the destination cannot be an SEV guest, and therefore can't be an SEV-ES guest either.

sev_guest() implies sev_es_guest() so I think this case is covered.

Quoted 4 lines

Quoted 2 lines
Fixes 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")
Cc [email protected]

Oops. I'll update in the V2 if needed. Added to this thread for now.

Sean Christopherson

maintainer

2021-09-14 18:41 UTC #3 lore raw

-stable, for giggles

Quoted 8 lines · Peter Gonda

Quoted 7 lines · Sean Christopherson
Quoted 4 lines · Peter Gonda
Copying an ASID into new vCPUs will not work for SEV-ES since the vCPUs VMSAs need to be setup and measured before SEV_LAUNCH_FINISH. Return an error if a users tries to KVM_CAP_VM_COPY_ENC_CONTEXT_FROM from an SEV-ES guest.

Quoted 8 lines

Quoted 2 lines
What happens if userspace does KVM_CAP_VM_COPY_ENC_CONTEXT_FROM before the source has created vCPUs, i.e. before it has done SEV_LAUNCH_FINISH?
That's not enough. If you wanted to be able to mirror SEV-ES you'd also need to call LAUNCH_UPDATE_VMSA on the mirror's vCPUs before SEV_LAUNCH_FINISH. That is do-able but I was writing a small change to fix this bug. If mirroring of SEV-ES is wanted it's a much bigger change.

Is it doable without KVM updates? If so, then outright rejection may not be the correct behavior.

Quoted 4 lines

Quoted 2 lines
Might be worth noting that the destination cannot be an SEV guest, and therefore can't be an SEV-ES guest either.
sev_guest() implies sev_es_guest() so I think this case is covered.

Yes, I was suggesting calling that out in the changelog so that readers/reviewers don't worry about that case.

Quoted 3 lines

Quoted 1 line
Cc [email protected]
Oops. I'll update in the V2 if needed. Added to this thread for now.

FWIW, you don't actually need to Cc stable, just including it in the changelog is sufficient as the script automagic will pick it up when it hits Linus' tree.

Peter Gonda

author

2021-09-14 18:47 UTC #4 lore raw

Quoted 15 lines · Sean Christopherson

-stable, for giggles
Quoted 11 lines · Peter Gonda
Quoted 9 lines · Sean Christopherson
Quoted 4 lines · Peter Gonda
Copying an ASID into new vCPUs will not work for SEV-ES since the vCPUs VMSAs need to be setup and measured before SEV_LAUNCH_FINISH. Return an error if a users tries to KVM_CAP_VM_COPY_ENC_CONTEXT_FROM from an SEV-ES guest.
What happens if userspace does KVM_CAP_VM_COPY_ENC_CONTEXT_FROM before the source has created vCPUs, i.e. before it has done SEV_LAUNCH_FINISH?

Quoted 8 lines

Quoted 5 lines
That's not enough. If you wanted to be able to mirror SEV-ES you'd also need to call LAUNCH_UPDATE_VMSA on the mirror's vCPUs before SEV_LAUNCH_FINISH. That is do-able but I was writing a small change to fix this bug. If mirroring of SEV-ES is wanted it's a much bigger change.
Is it doable without KVM updates? If so, then outright rejection may not be the correct behavior.

I do not think so. You cannot call KVM_SEV_LAUNCH_UPDATE_VMSA on the mirror because svm_mem_enc_op() blocks calls from the mirror. So either you have to update vmsa from the mirror or have the original VM read through its mirror's vCPUs when calling KVM_SEV_LAUNCH_UPDATE_VMSA. Not sure which way is better but I don't see a way to do this without updating KVM.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
Might be worth noting that the destination cannot be an SEV guest, and therefore can't be an SEV-ES guest either.
sev_guest() implies sev_es_guest() so I think this case is covered.
Yes, I was suggesting calling that out in the changelog so that readers/reviewers don't worry about that case.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Cc [email protected]
Oops. I'll update in the V2 if needed. Added to this thread for now.
FWIW, you don't actually need to Cc stable, just including it in the changelog is sufficient as the script automagic will pick it up when it hits Linus' tree.

Ack. I'll send out a V2 with updated changelog after we've settled on the first issue.

Sean Christopherson

maintainer

2021-09-14 18:49 UTC #5 lore raw

Quoted 22 lines · Peter Gonda

Quoted 21 lines · Sean Christopherson
-stable, for giggles
Quoted 16 lines · Peter Gonda
Quoted 9 lines · Sean Christopherson
Quoted 4 lines · Peter Gonda
Copying an ASID into new vCPUs will not work for SEV-ES since the vCPUs VMSAs need to be setup and measured before SEV_LAUNCH_FINISH. Return an error if a users tries to KVM_CAP_VM_COPY_ENC_CONTEXT_FROM from an SEV-ES guest.
What happens if userspace does KVM_CAP_VM_COPY_ENC_CONTEXT_FROM before the source has created vCPUs, i.e. before it has done SEV_LAUNCH_FINISH?
That's not enough. If you wanted to be able to mirror SEV-ES you'd also need to call LAUNCH_UPDATE_VMSA on the mirror's vCPUs before SEV_LAUNCH_FINISH. That is do-able but I was writing a small change to fix this bug. If mirroring of SEV-ES is wanted it's a much bigger change.

Quoted 8 lines

Quoted 2 lines
Is it doable without KVM updates? If so, then outright rejection may not be the correct behavior.
I do not think so. You cannot call KVM_SEV_LAUNCH_UPDATE_VMSA on the mirror because svm_mem_enc_op() blocks calls from the mirror. So either you have to update vmsa from the mirror or have the original VM read through its mirror's vCPUs when calling KVM_SEV_LAUNCH_UPDATE_VMSA. Not sure which way is better but I don't see a way to do this without updating KVM.

Ah, right, I forgot all of the SEV ioctls are blocked on the mirror. Put something to that effect into the changelog to squash any argument about whether or not this is the correct KVM behavior.

Thanks!

Paolo Bonzini

maintainer

2021-09-15 08:44 UTC #6 lore raw

Quoted 10 lines · Sean Christopherson

Quoted 5 lines · Peter Gonda
I do not think so. You cannot call KVM_SEV_LAUNCH_UPDATE_VMSA on the mirror because svm_mem_enc_op() blocks calls from the mirror. So either you have to update vmsa from the mirror or have the original VM read through its mirror's vCPUs when calling KVM_SEV_LAUNCH_UPDATE_VMSA. Not sure which way is better but I don't see a way to do this without updating KVM.
Ah, right, I forgot all of the SEV ioctls are blocked on the mirror. Put something to that effect into the changelog to squash any argument about whether or not this is the correct KVM behavior.

Indeed, at least KVM_SEV_LAUNCH_UPDATE_VMSA would have to be allowed in the mirror VM. Do you think anything else would be necessary?

Paolo

Peter Gonda

author

2021-09-15 16:10 UTC #7 lore raw

Quoted 9 lines · Paolo Bonzini

Quoted 7 lines · Sean Christopherson
Quoted 5 lines · Peter Gonda
I do not think so. You cannot call KVM_SEV_LAUNCH_UPDATE_VMSA on the mirror because svm_mem_enc_op() blocks calls from the mirror. So either you have to update vmsa from the mirror or have the original VM read through its mirror's vCPUs when calling KVM_SEV_LAUNCH_UPDATE_VMSA. Not sure which way is better but I don't see a way to do this without updating KVM.

Quoted 6 lines

Quoted 3 lines
Ah, right, I forgot all of the SEV ioctls are blocked on the mirror. Put something to that effect into the changelog to squash any argument about whether or not this is the correct KVM behavior.
Indeed, at least KVM_SEV_LAUNCH_UPDATE_VMSA would have to be allowed in the mirror VM. Do you think anything else would be necessary?

Thanks Paolo. Yes I think that only the KVM_SEV_LAUNCH_UPDATE_VMSA ioctl needs to be allowed on the mirror VM. But I don't think that's the only changes needed. Additionally the mirror VM will need the sev 'handle' and the sev device 'fd' copied in vm_vm_copy_asid_from(). The handle is needed for KVM_SEV_LAUNCH_UPDATE_VMSA, the fd is required for sev_issue_cmd(). Also you you'd need to mirror es_active bool. (I think its quite confusing that svm_vm_copy_asid_from() only copies some of the metadata in sev_info but I can see why as the locked pages and cg group metadata shouldn't be copied.) I *think* that would be all that's needed but I haven't tried or tested this in any way.

svm_vm_copy_asid_from() {

asid = to_kvm_svm(source_kvm)->sev_info.asid;
+ handle = to_kvm_svm(source_kvm)->sev_info.handle;
+ fd = to_kvm_svm(source_kvm)->sev_info.fd;
+ es_active = to_kvm_svm(source_kvm)->sev_info.es_active;

...

    /* 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->asid = asid;
    mirror_sev->active = true;

+ mirror_sev->handle = handle;
+ mirror_sev->fd = fd;
+ mirror_sev->es_active = es_active;

Paolo would you prefer a patch to enable ES mirroring or continue with this patch to disable it for now?

Quoted 3 lines

Paolo

Paolo Bonzini

maintainer

2021-09-15 22:33 UTC #8 lore raw

Quoted 18 lines · Peter Gonda

svm_vm_copy_asid_from() {
    asid = to_kvm_svm(source_kvm)->sev_info.asid;
+ handle = to_kvm_svm(source_kvm)->sev_info.handle;
+ fd = to_kvm_svm(source_kvm)->sev_info.fd;
+ es_active = to_kvm_svm(source_kvm)->sev_info.es_active;
...
     /* 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->asid = asid;
     mirror_sev->active = true;
(quoted diff) +3 -0
+  mirror_sev->handle = handle;
+  mirror_sev->fd = fd;
+ mirror_sev->es_active = es_active;

Quoted 2 lines

Paolo would you prefer a patch to enable ES mirroring or continue with this patch to disable it for now?

If it's possible to enable it, it would be better. The above would be a reasonable patch for 5.15-rc.

Paolo

Nathan Tempelman

2021-09-16 18:55 UTC #9 lore raw

Quoted 20 lines · Paolo Bonzini

Quoted 18 lines · Peter Gonda
svm_vm_copy_asid_from() {
    asid = to_kvm_svm(source_kvm)->sev_info.asid;
+ handle = to_kvm_svm(source_kvm)->sev_info.handle;
+ fd = to_kvm_svm(source_kvm)->sev_info.fd;
+ es_active = to_kvm_svm(source_kvm)->sev_info.es_active;
...
     /* 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->asid = asid;
     mirror_sev->active = true;
(quoted diff) +3 -0
+  mirror_sev->handle = handle;
+  mirror_sev->fd = fd;
+ mirror_sev->es_active = es_active;

Quoted 8 lines

Quoted 2 lines
Paolo would you prefer a patch to enable ES mirroring or continue with this patch to disable it for now?
If it's possible to enable it, it would be better. The above would be a reasonable patch for 5.15-rc.
Paolo

+1. We don't have any immediate plans for sev-es, but it would be nice to have while we're here. But if you want to make the trivial fix I can come along and do it later.

Nathan Tempelman

2021-09-16 19:06 UTC #10 lore raw

Quoted 28 lines · Nathan Tempelman

Quoted 26 lines · Paolo Bonzini
Quoted 18 lines · Peter Gonda
svm_vm_copy_asid_from() {
    asid = to_kvm_svm(source_kvm)->sev_info.asid;
+ handle = to_kvm_svm(source_kvm)->sev_info.handle;
+ fd = to_kvm_svm(source_kvm)->sev_info.fd;
+ es_active = to_kvm_svm(source_kvm)->sev_info.es_active;
...
     /* 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->asid = asid;
     mirror_sev->active = true;
(quoted diff) +3 -0
+  mirror_sev->handle = handle;
+  mirror_sev->fd = fd;
+ mirror_sev->es_active = es_active;
Quoted 2 lines
Paolo would you prefer a patch to enable ES mirroring or continue with this patch to disable it for now?
If it's possible to enable it, it would be better. The above would be a reasonable patch for 5.15-rc.

Quoted 6 lines

Quoted 2 lines
Paolo
+1. We don't have any immediate plans for sev-es, but it would be nice to have while we're here. But if you want to make the trivial fix I can come along and do it later.

+Steve Rutherford

Peter Gonda

author

2021-09-21 15:05 UTC #11 lore raw

Quoted 24 lines · Nathan Tempelman

Quoted 22 lines · Nathan Tempelman
Quoted 20 lines · Paolo Bonzini
Quoted 18 lines · Peter Gonda
svm_vm_copy_asid_from() {
    asid = to_kvm_svm(source_kvm)->sev_info.asid;
+ handle = to_kvm_svm(source_kvm)->sev_info.handle;
+ fd = to_kvm_svm(source_kvm)->sev_info.fd;
+ es_active = to_kvm_svm(source_kvm)->sev_info.es_active;
...
     /* 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->asid = asid;
     mirror_sev->active = true;
(quoted diff) +3 -0
+  mirror_sev->handle = handle;
+  mirror_sev->fd = fd;
+ mirror_sev->es_active = es_active;

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Paolo would you prefer a patch to enable ES mirroring or continue with this patch to disable it for now?
If it's possible to enable it, it would be better. The above would be a reasonable patch for 5.15-rc.
Paolo

Sounds good, sent a 2 patch series this morning.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
+1. We don't have any immediate plans for sev-es, but it would be nice to have while we're here. But if you want to make the trivial fix I can come along and do it later.
+Steve Rutherford