[PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[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.

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(-)

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;
 	}
-- 
2.33.0.309.g3052b89438-goog

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Sean Christopherson]

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

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Peter Gonda]

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.

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Sean Christopherson]

-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.

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Peter Gonda]

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.

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Sean Christopherson]

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!

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Paolo Bonzini]

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

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Peter Gonda]

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
>

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Paolo Bonzini]

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

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Nathan Tempelman]

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.

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Nathan Tempelman]

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

Re: [PATCH] KVM: SEV: Disable KVM_CAP_VM_COPY_ENC_CONTEXT_FROM for SEV-ES

[Peter Gonda]

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