Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.
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
> ---
> 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(-)
>
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.
Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.
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
Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.
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.
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.
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.
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
---
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
Re: [PATCH] KVM: SVM: Mark SEV launch secret pages as dirty.
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 Acked-by: David Rientjes
