| Message ID | 20200807012303.3769170-1-cfir@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Mark SEV launch secret pages as dirty. | expand |
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>
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>
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);
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
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. */
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
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; }
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(-)