| Message ID | 20210914171551.3223715-1-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES | expand |
On Tue, Sep 14, 2021, Peter Gonda wrote: > 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. > Fixes: 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context") Cc: stable@vger.kernel.org
On Tue, Sep 14, 2021 at 11:32 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Sep 14, 2021, Peter Gonda wrote: > > 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. > > 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. > > > Fixes: 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context") > > Cc: stable@vger.kernel.org Oops. I'll update in the V2 if needed. Added to this thread for now.
-stable, for giggles On Tue, Sep 14, 2021, Peter Gonda wrote: > On Tue, Sep 14, 2021 at 11:32 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Tue, Sep 14, 2021, Peter Gonda wrote: > > > 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. Is it doable without KVM updates? If so, then outright rejection may not be the correct behavior. > > 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. > > Cc: stable@vger.kernel.org > 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.
On Tue, Sep 14, 2021 at 12:41 PM Sean Christopherson <seanjc@google.com> wrote: > > -stable, for giggles > > On Tue, Sep 14, 2021, Peter Gonda wrote: > > On Tue, Sep 14, 2021 at 11:32 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Tue, Sep 14, 2021, Peter Gonda wrote: > > > > 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. > > 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. > > > > 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. > > > > Cc: stable@vger.kernel.org > > > 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.
On Tue, Sep 14, 2021, Peter Gonda wrote: > On Tue, Sep 14, 2021 at 12:41 PM Sean Christopherson <seanjc@google.com> wrote: > > > > -stable, for giggles > > > > On Tue, Sep 14, 2021, Peter Gonda wrote: > > > On Tue, Sep 14, 2021 at 11:32 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > > > On Tue, Sep 14, 2021, Peter Gonda wrote: > > > > > 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. > > > > 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!
On 14/09/21 20:49, Sean Christopherson wrote: > On Tue, Sep 14, 2021, Peter Gonda wrote: >> 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
On Wed, Sep 15, 2021 at 2:44 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 14/09/21 20:49, Sean Christopherson wrote: > > On Tue, Sep 14, 2021, Peter Gonda wrote: > >> 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? 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? > > Paolo >
On 15/09/21 18:10, Peter Gonda wrote: > 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? If it's possible to enable it, it would be better. The above would be a reasonable patch for 5.15-rc. Paolo
On Wed, Sep 15, 2021 at 3:33 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 15/09/21 18:10, Peter Gonda wrote: > > 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? > > 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.
On Thu, Sep 16, 2021 at 11:08 AM Nathan Tempelman <natet@google.com> wrote: > > On Wed, Sep 15, 2021 at 3:33 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > On 15/09/21 18:10, Peter Gonda wrote: > > > 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? > > > > 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. +Steve Rutherford
On Thu, Sep 16, 2021 at 1:00 PM Nathan Tempelman <natet@google.com> wrote: > > On Thu, Sep 16, 2021 at 11:08 AM Nathan Tempelman <natet@google.com> wrote: > > > > On Wed, Sep 15, 2021 at 3:33 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > On 15/09/21 18:10, Peter Gonda wrote: > > > > 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? > > > > > > 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. > > > > > > > +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
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 75e0b21ad07c..8a279027425f 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -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; }
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 <pgonda@google.com> Cc: Marc Orr <marcorr@google.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Sean Christopherson <seanjc@google.com> Cc: Nathan Tempelman <natet@google.com> Cc: Brijesh Singh <brijesh.singh@amd.com> Cc: kvm@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- arch/x86/kvm/svm/sev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)