[PATCH 0/6] KVM: SEV: Bug fix, cleanups and enhancements

[PATCH 0/6] KVM: SEV: Bug fix, cleanups and enhancements

[Sean Christopherson]

Bug fix for COPY_ENC_CONTEXT_FROM that IIRC is very belated feedback (git
says its been sitting in my local repo since at least early September).

The other patches are tangentially related cleanups and enhancements for
the SEV and SEV-ES info, e.g. active flag, ASID, etc...

Booted an SEV guest, otherwise it's effectively all compile-tested only.

Sean Christopherson (6):
  KVM: SEV: Disallow COPY_ENC_CONTEXT_FROM if target has created vCPUs
  KVM: SEV: Explicitly document that there are no TOCTOU races in copy
    ASID
  KVM: SEV: Set sev_info.active after initial checks in sev_guest_init()
  KVM: SEV: WARN if SEV-ES is marked active but SEV is not
  KVM: SEV: Drop a redundant setting of sev->asid during initialization
  KVM: SEV: Fix typo in and tweak name of cmd_allowed_from_miror()

 arch/x86/kvm/svm/sev.c | 42 +++++++++++++++++++++++++++---------------
 arch/x86/kvm/svm/svm.h |  2 +-
 2 files changed, 28 insertions(+), 16 deletions(-)

-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH 1/6] KVM: SEV: Disallow COPY_ENC_CONTEXT_FROM if target has created vCPUs

[Sean Christopherson]

Reject COPY_ENC_CONTEXT_FROM if the destination VM has created vCPUs.
KVM relies on SEV activation to occur before vCPUs are created, e.g. to
set VMCB flags and intercepts correctly.

Fixes: 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")
Cc: stable@vger.kernel.org
Cc: Peter Gonda <pgonda@google.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Nathan Tempelman <natet@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 3e2769855e51..eeec499e4372 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1775,7 +1775,12 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
 	mutex_unlock(&source_kvm->lock);
 	mutex_lock(&kvm->lock);
 
-	if (sev_guest(kvm)) {
+	/*
+	 * 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;
 	}
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH 1/6] KVM: SEV: Disallow COPY_ENC_CONTEXT_FROM if target has created vCPUs

[Peter Gonda]

On Tue, Nov 9, 2021 at 2:53 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Reject COPY_ENC_CONTEXT_FROM if the destination VM has created vCPUs.
> KVM relies on SEV activation to occur before vCPUs are created, e.g. to
> set VMCB flags and intercepts correctly.
>
> Fixes: 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")
> Cc: stable@vger.kernel.org
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Nathan Tempelman <natet@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 3e2769855e51..eeec499e4372 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1775,7 +1775,12 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>         mutex_unlock(&source_kvm->lock);
>         mutex_lock(&kvm->lock);
>
> -       if (sev_guest(kvm)) {
> +       /*
> +        * 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;
>         }

Now that we have some framework for running SEV related selftests, do
you mind adding a regression test for this change?

> --
> 2.34.0.rc0.344.g81b53c2807-goog
>

Re: [PATCH 1/6] KVM: SEV: Disallow COPY_ENC_CONTEXT_FROM if target has created vCPUs

[Sean Christopherson]

On Mon, Nov 15, 2021, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 2:53 PM Sean Christopherson <seanjc@google.com> wrote:
> > +       /*
> > +        * 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;
> >         }
> 
> Now that we have some framework for running SEV related selftests, do
> you mind adding a regression test for this change?

Can do, will likely be a few days though.

[PATCH 2/6] KVM: SEV: Explicitly document that there are no TOCTOU races in copy ASID

[Sean Christopherson]

Deliberately grab the source's SEV info for COPY_ENC_CONTEXT_FROM outside
of kvm->lock and document that doing so is safe due to SEV/SEV-ES info,
e.g. ASID, active, etc... being "write-once" and set atomically with
respect to kvm->lock.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index eeec499e4372..6d14e2595c96 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1737,9 +1737,9 @@ int svm_unregister_enc_region(struct kvm *kvm,
 
 int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
 {
+	struct kvm_sev_info *mirror_sev, *source_sev;
 	struct file *source_kvm_file;
 	struct kvm *source_kvm;
-	struct kvm_sev_info source_sev, *mirror_sev;
 	int ret;
 
 	source_kvm_file = fget(source_fd);
@@ -1762,9 +1762,6 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
 		goto e_source_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
@@ -1785,14 +1782,25 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
 		goto e_mirror_unlock;
 	}
 
+	/*
+	 * Referencing the source's sev_info without holding the source's lock
+	 * is safe as SEV/SEV-ES activation is a one-way, "atomic" operation.
+	 * SEV state, e.g. the ASID, is modified under kvm->lock, and cannot be
+	 * changed after SEV is marked active (here or in normal activation).
+	 * That same atomicity also prevents TOC-TOU issues with respect to
+	 * related sanity checks on source_kvm.
+	 */
+	source_sev = &to_kvm_svm(source_kvm)->sev_info;
+
 	/* 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 = source_sev->asid;
 	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;
 	/*
 	 * Do not copy ap_jump_table. Since the mirror does not share the same
 	 * KVM contexts as the original, and they may have different
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH 2/6] KVM: SEV: Explicitly document that there are no TOCTOU races in copy ASID

[Paolo Bonzini]

On 11/9/21 22:50, Sean Christopherson wrote:
> Deliberately grab the source's SEV info for COPY_ENC_CONTEXT_FROM outside
> of kvm->lock and document that doing so is safe due to SEV/SEV-ES info,
> e.g. ASID, active, etc... being "write-once" and set atomically with
> respect to kvm->lock.
> 
> No functional change intended.

This isn't true anymore with the move-enc-context-from patches, though! 
  I will send a follow-up shortly.

Paolo

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/sev.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index eeec499e4372..6d14e2595c96 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1737,9 +1737,9 @@ int svm_unregister_enc_region(struct kvm *kvm,
>   
>   int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>   {
> +	struct kvm_sev_info *mirror_sev, *source_sev;
>   	struct file *source_kvm_file;
>   	struct kvm *source_kvm;
> -	struct kvm_sev_info source_sev, *mirror_sev;
>   	int ret;
>   
>   	source_kvm_file = fget(source_fd);
> @@ -1762,9 +1762,6 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>   		goto e_source_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
> @@ -1785,14 +1782,25 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>   		goto e_mirror_unlock;
>   	}
>   
> +	/*
> +	 * Referencing the source's sev_info without holding the source's lock
> +	 * is safe as SEV/SEV-ES activation is a one-way, "atomic" operation.
> +	 * SEV state, e.g. the ASID, is modified under kvm->lock, and cannot be
> +	 * changed after SEV is marked active (here or in normal activation).
> +	 * That same atomicity also prevents TOC-TOU issues with respect to
> +	 * related sanity checks on source_kvm.
> +	 */
> +	source_sev = &to_kvm_svm(source_kvm)->sev_info;
> +
>   	/* 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 = source_sev->asid;
>   	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;
>   	/*
>   	 * Do not copy ap_jump_table. Since the mirror does not share the same
>   	 * KVM contexts as the original, and they may have different
> 

[PATCH 3/6] KVM: SEV: Set sev_info.active after initial checks in sev_guest_init()

[Sean Christopherson]

Set sev_info.active during SEV/SEV-ES activation before calling any code
that can potentially consume sev_info.es_active, e.g. set "active" and
"es_active" as a pair immediately after the initial sanity checks.  KVM
generally expects that es_active can be true if and only if active is
true, e.g. sev_asid_new() deliberately avoids sev_es_guest() so that it
doesn't get a false negative.  This will allow WARNing in sev_es_guest()
if the VM is tagged as SEV-ES but not SEV.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 6d14e2595c96..a869b11301df 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -229,7 +229,6 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	bool es_active = argp->id == KVM_SEV_ES_INIT;
 	int asid, ret;
 
 	if (kvm->created_vcpus)
@@ -239,7 +238,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (unlikely(sev->active))
 		return ret;
 
-	sev->es_active = es_active;
+	sev->active = true;
+	sev->es_active = argp->id == KVM_SEV_ES_INIT;
 	asid = sev_asid_new(sev);
 	if (asid < 0)
 		goto e_no_asid;
@@ -249,7 +249,6 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (ret)
 		goto e_free;
 
-	sev->active = true;
 	sev->asid = asid;
 	INIT_LIST_HEAD(&sev->regions_list);
 
@@ -260,6 +259,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	sev->asid = 0;
 e_no_asid:
 	sev->es_active = false;
+	sev->active = false;
 	return ret;
 }
 
-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH 4/6] KVM: SEV: WARN if SEV-ES is marked active but SEV is not

[Sean Christopherson]

WARN if the VM is tagged as SEV-ES but not SEV.  KVM relies on SEV and
SEV-ES being set atomically, and guards common flows with "is SEV", i.e.
observing SEV-ES without SEV means KVM has a fatal bug.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 0d7bbe548ac3..a345f557be4a 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -242,7 +242,7 @@ static inline bool sev_es_guest(struct kvm *kvm)
 #ifdef CONFIG_KVM_AMD_SEV
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 
-	return sev_guest(kvm) && sev->es_active;
+	return sev->es_active && !WARN_ON_ONCE(!sev->active);
 #else
 	return false;
 #endif
-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH 5/6] KVM: SEV: Drop a redundant setting of sev->asid during initialization

[Sean Christopherson]

Remove a fully redundant write to sev->asid during SEV/SEV-ES guest
initialization.  The ASID is set a few lines earlier prior to the call to
sev_platform_init(), which doesn't take "sev" as a param, i.e. can't
muck with the ASID barring some truly magical behind-the-scenes code.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a869b11301df..a69dfa0d62aa 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -249,7 +249,6 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (ret)
 		goto e_free;
 
-	sev->asid = asid;
 	INIT_LIST_HEAD(&sev->regions_list);
 
 	return 0;
-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH 6/6] KVM: SEV: Fix typo in and tweak name of cmd_allowed_from_miror()

[Sean Christopherson]

Rename cmd_allowed_from_miror() => yield is_cmd_allowed_from_mirror() to
fix a typo and to make it obvious that the result is a boolean where
false means "not allowed".

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a69dfa0d62aa..2b891509251a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1509,7 +1509,7 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return sev_issue_cmd(kvm, SEV_CMD_RECEIVE_FINISH, &data, &argp->error);
 }
 
-static bool cmd_allowed_from_miror(u32 cmd_id)
+static bool is_cmd_allowed_from_mirror(u32 cmd_id)
 {
 	/*
 	 * Allow mirrors VM to call KVM_SEV_LAUNCH_UPDATE_VMSA to enable SEV-ES
@@ -1541,7 +1541,7 @@ int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 
 	/* Only the enc_context_owner handles some memory enc operations. */
 	if (is_mirroring_enc_context(kvm) &&
-	    !cmd_allowed_from_miror(sev_cmd.id)) {
+	    !is_cmd_allowed_from_mirror(sev_cmd.id)) {
 		r = -EINVAL;
 		goto out;
 	}
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH 0/6] KVM: SEV: Bug fix, cleanups and enhancements

[Paolo Bonzini]

On 11/9/21 22:50, Sean Christopherson wrote:
> Bug fix for COPY_ENC_CONTEXT_FROM that IIRC is very belated feedback (git
> says its been sitting in my local repo since at least early September).
> 
> The other patches are tangentially related cleanups and enhancements for
> the SEV and SEV-ES info, e.g. active flag, ASID, etc...
> 
> Booted an SEV guest, otherwise it's effectively all compile-tested only.
> 
> Sean Christopherson (6):
>    KVM: SEV: Disallow COPY_ENC_CONTEXT_FROM if target has created vCPUs
>    KVM: SEV: Explicitly document that there are no TOCTOU races in copy
>      ASID
>    KVM: SEV: Set sev_info.active after initial checks in sev_guest_init()
>    KVM: SEV: WARN if SEV-ES is marked active but SEV is not
>    KVM: SEV: Drop a redundant setting of sev->asid during initialization
>    KVM: SEV: Fix typo in and tweak name of cmd_allowed_from_miror()
> 
>   arch/x86/kvm/svm/sev.c | 42 +++++++++++++++++++++++++++---------------
>   arch/x86/kvm/svm/svm.h |  2 +-
>   2 files changed, 28 insertions(+), 16 deletions(-)
> 

Queued all except 2.

Paolo