[PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Cfir Cohen]

The LAUNCH_SECRET command performs encryption of the
launch secret memory contents. Mark pinned pages as
dirty, before unpinning them.
This matches the logic in sev_launch_update().

Signed-off-by: Cfir Cohen <cfir@google.com>
---
 arch/x86/kvm/svm/sev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5573a97f1520..37c47d26b9f7 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -850,7 +850,7 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	struct kvm_sev_launch_secret params;
 	struct page **pages;
 	void *blob, *hdr;
-	unsigned long n;
+	unsigned long n, i;
 	int ret, offset;
 
 	if (!sev_guest(kvm))
@@ -863,6 +863,14 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (!pages)
 		return -ENOMEM;
 
+	/*
+	 * The LAUNCH_SECRET command will perform in-place encryption of the
+	 * memory content (i.e it will write the same memory region with C=1).
+	 * It's possible that the cache may contain the data with C=0, i.e.,
+	 * unencrypted so invalidate it first.
+	 */
+	sev_clflush_pages(pages, n);
+
 	/*
 	 * The secret must be copied into contiguous memory region, lets verify
 	 * that userspace memory pages are contiguous before we issue command.
@@ -908,6 +916,11 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 e_free:
 	kfree(data);
 e_unpin_memory:
+	/* content of memory is updated, mark pages dirty */
+	for (i = 0; i < n; i++) {
+		set_page_dirty_lock(pages[i]);
+		mark_page_accessed(pages[i]);
+	}
 	sev_unpin_memory(kvm, pages, n);
 	return ret;
 }
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[David Rientjes]

On Thu, 6 Aug 2020, Cfir Cohen wrote:

> The LAUNCH_SECRET command performs encryption of the
> launch secret memory contents. Mark pinned pages as
> dirty, before unpinning them.
> This matches the logic in sev_launch_update().
> 
> Signed-off-by: Cfir Cohen <cfir@google.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Krish Sadhukhan]

On 8/6/20 6:23 PM, Cfir Cohen wrote:
> The LAUNCH_SECRET command performs encryption of the
> launch secret memory contents. Mark pinned pages as
> dirty, before unpinning them.
> This matches the logic in sev_launch_update().
sev_launch_update_data() instead of sev_launch_update() ?
>
> Signed-off-by: Cfir Cohen <cfir@google.com>
> ---
>   arch/x86/kvm/svm/sev.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5573a97f1520..37c47d26b9f7 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -850,7 +850,7 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	struct kvm_sev_launch_secret params;
>   	struct page **pages;
>   	void *blob, *hdr;
> -	unsigned long n;
> +	unsigned long n, i;
>   	int ret, offset;
>   
>   	if (!sev_guest(kvm))
> @@ -863,6 +863,14 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	if (!pages)
>   		return -ENOMEM;
>   
> +	/*
> +	 * The LAUNCH_SECRET command will perform in-place encryption of the
> +	 * memory content (i.e it will write the same memory region with C=1).
> +	 * It's possible that the cache may contain the data with C=0, i.e.,
> +	 * unencrypted so invalidate it first.
> +	 */
> +	sev_clflush_pages(pages, n);
> +
>   	/*
>   	 * The secret must be copied into contiguous memory region, lets verify
>   	 * that userspace memory pages are contiguous before we issue command.
> @@ -908,6 +916,11 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   e_free:
>   	kfree(data);
>   e_unpin_memory:
> +	/* content of memory is updated, mark pages dirty */
> +	for (i = 0; i < n; i++) {
> +		set_page_dirty_lock(pages[i]);
> +		mark_page_accessed(pages[i]);
> +	}
>   	sev_unpin_memory(kvm, pages, n);
>   	return ret;
>   }
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

[PATCH v2] KVM: SVM: Mark SEV launch secret pages as dirty.

[Cfir Cohen]

The LAUNCH_SECRET command performs encryption of the
launch secret memory contents. Mark pinned pages as
dirty, before unpinning them.
This matches the logic in sev_launch_update_data().

Signed-off-by: Cfir Cohen <cfir@google.com>
---
Changelog since v1:
 - Updated commit message.

 arch/x86/kvm/svm/sev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5573a97f1520..37c47d26b9f7 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -850,7 +850,7 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	struct kvm_sev_launch_secret params;
 	struct page **pages;
 	void *blob, *hdr;
-	unsigned long n;
+	unsigned long n, i;
 	int ret, offset;
 
 	if (!sev_guest(kvm))
@@ -863,6 +863,14 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (!pages)
 		return -ENOMEM;
 
+	/*
+	 * The LAUNCH_SECRET command will perform in-place encryption of the
+	 * memory content (i.e it will write the same memory region with C=1).
+	 * It's possible that the cache may contain the data with C=0, i.e.,
+	 * unencrypted so invalidate it first.
+	 */
+	sev_clflush_pages(pages, n);
+
 	/*
 	 * The secret must be copied into contiguous memory region, lets verify
 	 * that userspace memory pages are contiguous before we issue command.
@@ -908,6 +916,11 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 e_free:
 	kfree(data);
 e_unpin_memory:
+	/* content of memory is updated, mark pages dirty */
+	for (i = 0; i < n; i++) {
+		set_page_dirty_lock(pages[i]);
+		mark_page_accessed(pages[i]);
+	}
 	sev_unpin_memory(kvm, pages, n);
 	return ret;
 }
-- 
2.28.0.236.gb10cc79966-goog

Re: [PATCH v2] KVM: SVM: Mark SEV launch secret pages as dirty.

[Brijesh Singh]

On 8/7/20 7:37 PM, Cfir Cohen wrote:
> The LAUNCH_SECRET command performs encryption of the
> launch secret memory contents. Mark pinned pages as
> dirty, before unpinning them.
> This matches the logic in sev_launch_update_data().
>
> Signed-off-by: Cfir Cohen <cfir@google.com>
> ---
> Changelog since v1:
>  - Updated commit message.
>
>  arch/x86/kvm/svm/sev.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)


Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>


>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5573a97f1520..37c47d26b9f7 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -850,7 +850,7 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	struct kvm_sev_launch_secret params;
>  	struct page **pages;
>  	void *blob, *hdr;
> -	unsigned long n;
> +	unsigned long n, i;
>  	int ret, offset;
>  
>  	if (!sev_guest(kvm))
> @@ -863,6 +863,14 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (!pages)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The LAUNCH_SECRET command will perform in-place encryption of the
> +	 * memory content (i.e it will write the same memory region with C=1).
> +	 * It's possible that the cache may contain the data with C=0, i.e.,
> +	 * unencrypted so invalidate it first.
> +	 */
> +	sev_clflush_pages(pages, n);
> +
>  	/*
>  	 * The secret must be copied into contiguous memory region, lets verify
>  	 * that userspace memory pages are contiguous before we issue command.
> @@ -908,6 +916,11 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  e_free:
>  	kfree(data);
>  e_unpin_memory:
> +	/* content of memory is updated, mark pages dirty */
> +	for (i = 0; i < n; i++) {
> +		set_page_dirty_lock(pages[i]);
> +		mark_page_accessed(pages[i]);
> +	}
>  	sev_unpin_memory(kvm, pages, n);
>  	return ret;
>  }

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Paolo Bonzini]

On 19/09/20 06:55, Sean Christopherson wrote:
> Side topic, while I love the comment (I do, honestly) regarding in-place
> encryption, this is the fourth? instance of the same 4-line comment (6 lines
> if you count the /* and */.  Maybe it's time to do something like
> 
> 	/* LAUNCH_SECRET does in-place encryption, see sev_clflush_pages(). */
> 
> and then have the main comment in sev_clflush_pages().  With the addition of
> X86_FEATURE_SME_COHERENT, there's even a fantastic location for the comment:

Two of the three instances are a bit different though.  What about this
which at least shortens the comment to 2 fewer lines:

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index bb0e89c79a04..7b11546e65ba 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -446,10 +446,8 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 	/*
-	 * The LAUNCH_UPDATE command will perform in-place encryption of the
-	 * memory content (i.e it will write the same memory region with C=1).
-	 * It's possible that the cache may contain the data with C=0, i.e.,
-	 * unencrypted so invalidate it first.
+	 * Flush before LAUNCH_UPDATE encrypts pages in place, in case the cache
+	 * contains the data that was written unencrypted.
 	 */
 	sev_clflush_pages(inpages, npages);
 
@@ -805,10 +803,8 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
 		}
 
 		/*
-		 * The DBG_{DE,EN}CRYPT commands will perform {dec,en}cryption of the
-		 * memory content (i.e it will write the same memory region with C=1).
-		 * It's possible that the cache may contain the data with C=0, i.e.,
-		 * unencrypted so invalidate it first.
+		 * Flush before DBG_{DE,EN}CRYPT reads or modifies the pages, flush the
+		 * destination too in case the cache contains its current data.
 		 */
 		sev_clflush_pages(src_p, 1);
 		sev_clflush_pages(dst_p, 1);
@@ -870,10 +866,8 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		return PTR_ERR(pages);
 
 	/*
-	 * The LAUNCH_SECRET command will perform in-place encryption of the
-	 * memory content (i.e it will write the same memory region with C=1).
-	 * It's possible that the cache may contain the data with C=0, i.e.,
-	 * unencrypted so invalidate it first.
+	 * Flush before LAUNCH_SECRET encrypts pages in place, in case the cache
+	 * contains the data that was written unencrypted.
 	 */
 	sev_clflush_pages(pages, n);
 

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Paolo Bonzini]

On 23/09/20 19:04, Sean Christopherson wrote:
>> Two of the three instances are a bit different though.  What about this
>> which at least shortens the comment to 2 fewer lines:
> Any objection to changing those to "Flush (on non-coherent CPUs)"?  I agree
> it would be helpful to call out the details, especially for DBG_*, but I
> don't like that it reads as if the flush is unconditional.

Hmm... It's already fairly long lines so that would wrap to 3 lines, and
the reference to the conditional flush wasn't there before either.

sev_clflush_pages could be a better place to mention that (or perhaps
it's self-explanatory).

Paolo

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Sean Christopherson]

On Wed, Sep 23, 2020 at 07:16:08PM +0200, Paolo Bonzini wrote:
> On 23/09/20 19:04, Sean Christopherson wrote:
> >> Two of the three instances are a bit different though.  What about this
> >> which at least shortens the comment to 2 fewer lines:
> > Any objection to changing those to "Flush (on non-coherent CPUs)"?  I agree
> > it would be helpful to call out the details, especially for DBG_*, but I
> > don't like that it reads as if the flush is unconditional.
> 
> Hmm... It's already fairly long lines so that would wrap to 3 lines, and

Dang, I was hoping it would squeeze into 2.

> the reference to the conditional flush wasn't there before either.

Well, the flush wasn't conditional before (ignoring the NULL check).
 
> sev_clflush_pages could be a better place to mention that (or perhaps
> it's self-explanatory).

I agree, but with

	/*
	 * Flush before LAUNCH_UPDATE encrypts pages in place, in case the cache
	 * contains the data that was written unencrypted.
 	 */
 	sev_clflush_pages(inpages, npages);

there's nothing in the comment or code that even suggests sev_clflush_pages() is
conditional, i.e. no reason for the reader to peek at the implemenation.

What about:

	/*
	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
	 * place, the cache may contain data that was written unencrypted.
	 */

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Paolo Bonzini]

On 23/09/20 19:26, Sean Christopherson wrote:
> 	/*
> 	 * Flush before LAUNCH_UPDATE encrypts pages in place, in case the cache
> 	 * contains the data that was written unencrypted.
>  	 */
>  	sev_clflush_pages(inpages, npages);
> 
> there's nothing in the comment or code that even suggests sev_clflush_pages() is
> conditional, i.e. no reason for the reader to peek at the implemenation.
> 
> What about:
> 
> 	/*
> 	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
> 	 * place, the cache may contain data that was written unencrypted.
> 	 */

Sounds good.

Paolo

[PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Cfir Cohen]

The LAUNCH_SECRET command performs encryption of the
launch secret memory contents. Mark pinned pages as
dirty, before unpinning them.
This matches the logic in sev_launch_update_data().

Fixes: 9c5e0afaf157 ("KVM: SVM: Add support for SEV LAUNCH_SECRET command")
Signed-off-by: Cfir Cohen <cfir@google.com>
---
Changelog since v2:
 - Added 'Fixes' tag, updated comments.
Changelog since v1:
 - Updated commit message.

 arch/x86/kvm/svm/sev.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5573a97f1520..55edaf3577a0 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -440,10 +440,8 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 	/*
-	 * The LAUNCH_UPDATE command will perform in-place encryption of the
-	 * memory content (i.e it will write the same memory region with C=1).
-	 * It's possible that the cache may contain the data with C=0, i.e.,
-	 * unencrypted so invalidate it first.
+	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
+	 * place, the cache may contain data that was written unencrypted.
 	 */
 	sev_clflush_pages(inpages, npages);
 
@@ -799,10 +797,9 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
 		}
 
 		/*
-		 * The DBG_{DE,EN}CRYPT commands will perform {dec,en}cryption of the
-		 * memory content (i.e it will write the same memory region with C=1).
-		 * It's possible that the cache may contain the data with C=0, i.e.,
-		 * unencrypted so invalidate it first.
+		 * Flush (on non-coherent CPUs) before DBG_{DE,EN}CRYPT reads or modifies
+		 * the pages, flush the destination too in case the cache contains its
+		 * current data.
 		 */
 		sev_clflush_pages(src_p, 1);
 		sev_clflush_pages(dst_p, 1);
@@ -850,7 +847,7 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	struct kvm_sev_launch_secret params;
 	struct page **pages;
 	void *blob, *hdr;
-	unsigned long n;
+	unsigned long n, i;
 	int ret, offset;
 
 	if (!sev_guest(kvm))
@@ -863,6 +860,12 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (!pages)
 		return -ENOMEM;
 
+	/*
+	 * Flush (on non-coherent CPUs) before LAUNCH_SECRET encrypts pages in
+	 * place, the cache may contain data that was written unencrypted.
+	 */
+	sev_clflush_pages(pages, n);
+
 	/*
 	 * The secret must be copied into contiguous memory region, lets verify
 	 * that userspace memory pages are contiguous before we issue command.
@@ -908,6 +911,11 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 e_free:
 	kfree(data);
 e_unpin_memory:
+	/* content of memory is updated, mark pages dirty */
+	for (i = 0; i < n; i++) {
+		set_page_dirty_lock(pages[i]);
+		mark_page_accessed(pages[i]);
+	}
 	sev_unpin_memory(kvm, pages, n);
 	return ret;
 }
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.

[Greg KH]

On Thu, Sep 24, 2020 at 07:00:11PM -0700, Cfir Cohen wrote:
> The LAUNCH_SECRET command performs encryption of the
> launch secret memory contents. Mark pinned pages as
> dirty, before unpinning them.
> This matches the logic in sev_launch_update_data().
> 
> Fixes: 9c5e0afaf157 ("KVM: SVM: Add support for SEV LAUNCH_SECRET command")
> Signed-off-by: Cfir Cohen <cfir@google.com>
> ---
> Changelog since v2:
>  - Added 'Fixes' tag, updated comments.
> Changelog since v1:
>  - Updated commit message.
> 
>  arch/x86/kvm/svm/sev.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>