All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Michael Roth <michael.roth@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/6] KVM: selftests: Add a test to verify SEV {en,de}crypt debug ioctls
Date: Wed, 24 Jun 2026 17:25:07 -0700	[thread overview]
Message-ID: <ajx1Y93iX2oV-DLa@google.com> (raw)
In-Reply-To: <45ixyum4upln4b2yvasovm2bmsfxyfezsi23vsr75r4fklelef@utxv4mymvfcd>

On Wed, Jun 24, 2026, Michael Roth wrote:
> On Wed, Jun 24, 2026 at 02:24:24PM -0700, Sean Christopherson wrote:
> > On Tue, Jun 23, 2026, Sean Christopherson wrote:
> > > On Tue, Jun 23, 2026, Michael Roth wrote:
> > > > On Tue, Jun 23, 2026 at 07:55:27PM +0000, Sean Christopherson wrote:
> > > > since your proposed fix seems nicer and more complete anyway.
> > > 
> > > It's missing a case though :-(
> > 
> > Ah, but so is the revert, and it's not a coincidence that the old code didn't
> > bounce buffer the destination for KVM_SEV_DBG_ENCRYPT.
> 
> Yah I was realizing this too :( Though I'm not sure why we set
> 'guest_owned' for DBG_DECRYPT... I'm not even sure it makes sense to
> write decrypted data into guest memory since it would only be usable
> if the range was decrypted to begin with... in most cases this likely
> wouldn't be guest memory.
> 
> So maybe the bounce buffer approach would work for DBG_DECRYPT at
> least,

Yeah, DECRYPT worked fine, it's all the others that break.

> > pread() doesn't work, because the source or destination needs to be a userspace
> > address, and so any RMP #PFs will get eaten.  But copy_file_range() makes the
> > world go kablooie.
> 
> I was hoping we could at least rely on kernel accesses going through GUP
> and maybe doing something like unmapping and freezing folio refcount
> to block concurrent access to the page, or read-only protections that
> would force the GUP to fail and just fail the concurrent userspace-requested
> operation...
> 
> copy_file_range() bypasses the GUP, but freezing the refcount or locking
> the folio would cause it to spin until the folio was released. So maybe
> that's an option?
> 
> This is similar to what's been proposed for gmem hugetlb support when
> splitting/rebuilding hugepages as part of the conversion path, where
> we need to guard against concurrent access by both kernel/userspace, so
> maybe it's not too hacky?

The big difference is that guest_memfd owns the folio.  For SEV/SEV-ES, KVM has
no clue what sits behind the destination of the ENCRYPT operation (or the other
guest_owned commands).  KVM knows it's a refcounted struct page due to pinning
the memory, but that's about it.  We'd basically have to audit every filesystem
to see if a "fix" would actually work.

And the risk isn't limited to copy_file_range(), that just happens to be the first
syscall I found that did what I want.  E.g. any memory that is kernel allocated
and mapped into userspace would be problematic, because that memory could be fed
back into KVM.  Ha!  Now that I think about it, I bet the pages mapped via
kvm_vcpu_fault() are problematic, because KVM owns the pages and will access them
at will.

So I don't think we can fix this by playing games in KVM.  Which is pretty much
why we ended up with guest_memfd: the only way to lock down memory to the point
where we're 100% certain it's safe to "poison" the pages is if KVM sequesters
them away in a dedicated file system.

> > I don't see a way to salvage SEV/SEV-ES on SNP systems, short of requiring
> > CAP_SYS_BOOT or some other elevated permission to do *anything*.  Or redo SEV/SEV-ES
> > support to require guest_memfd for operations that require putting pages into
> > Firmware state.
> 
> Yah, short of maybe the above approach, I don't see any way around it atm :(
> If you think it's worth pursuing though I can give that a shot on my
> end.

I say we wait for Paolo to get back from holiday (in a few weeks) before doing
anything drastic.  I'll send a patch to fix the existing selftest though, no
reason to leave that hole open.

  reply	other threads:[~2026-06-25  0:25 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-01 20:35 [PATCH v2 0/6] KVM: SEV: sev_dbg_crypt() fix and overhaul Sean Christopherson
2026-05-01 20:35 ` [PATCH v2 1/6] KVM: SVM: Fix page overflow in sev_dbg_crypt() for ENCRYPT path Sean Christopherson
2026-05-01 20:35 ` [PATCH v2 2/6] KVM: selftests: Add a test to verify SEV {en,de}crypt debug ioctls Sean Christopherson
2026-05-21  7:04   ` [PATCH v2 2/6] KVM: selftests: Add a test to verify SEV {en,de}crypt kernel test robot
2026-05-21 14:28     ` Sean Christopherson
     [not found]   ` <ajNQ_Ix5e0p6FuFT@google.com>
     [not found]     ` <tx3hcnrhfbcz2p7e7umc5o2zrw74pluzuzgtlqt4dz5cc5nhxx@6tzzorc25nqw>
     [not found]       ` <s7jfbw3tnfgmp6blfqlfoi7yhq6f5n6xl62zzty5up45cqtsty@ljpjlvqw2gyy>
2026-06-23 19:55         ` [PATCH v2 2/6] KVM: selftests: Add a test to verify SEV {en,de}crypt debug ioctls Sean Christopherson
2026-06-23 23:22           ` Michael Roth
2026-06-24  0:22             ` Sean Christopherson
2026-06-24 21:24               ` Sean Christopherson
2026-06-24 23:11                 ` Michael Roth
2026-06-25  0:25                   ` Sean Christopherson [this message]
2026-05-01 20:35 ` [PATCH v2 3/6] KVM: SEV: Explicitly validate the dst buffer for debug operations Sean Christopherson
2026-05-01 20:35 ` [PATCH v2 4/6] KVM: SEV: Add helper function to pin/unpin a single page Sean Christopherson
2026-05-01 20:35 ` [PATCH v2 5/6] KVM: SEV: Rewrite logic to {de,en}crypt memory for debug Sean Christopherson
2026-05-01 20:35 ` [PATCH v2 6/6] KVM: SEV: Allocate only as many bytes as needed for temp crypt buffers Sean Christopherson
2026-05-19  0:41 ` [PATCH v2 0/6] KVM: SEV: sev_dbg_crypt() fix and overhaul Sean Christopherson
  -- strict thread matches above, loose matches on Subject: below --
2026-04-16 23:10 Sean Christopherson
2026-04-16 23:10 ` [PATCH v2 2/6] KVM: selftests: Add a test to verify SEV {en,de}crypt debug ioctls Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ajx1Y93iX2oV-DLa@google.com \
    --to=seanjc@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michael.roth@amd.com \
    --cc=pbonzini@redhat.com \
    --cc=thomas.lendacky@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.