Message-Id: <20231017202505.340906-1-rick.p.edgecombe@intel.com>
Date:   Tue, 17 Oct 2023 13:24:55 -0700
From:   Rick Edgecombe <rick.p.edgecombe@...el.com>
To:     x86@...nel.org, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, luto@...nel.org,
        peterz@...radead.org, kirill.shutemov@...ux.intel.com,
        elena.reshetova@...el.com, isaku.yamahata@...el.com,
        seanjc@...gle.com, Michael Kelley <mikelley@...rosoft.com>,
        thomas.lendacky@....com, decui@...rosoft.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org
Cc:     rick.p.edgecombe@...el.com
Subject: [PATCH 00/10] Handle set_memory_XXcrypted() errors

Shared pages should never return to the page allocator, or future usage of
the pages may allow for the contents to be exposed to the host. They may
also cause the guest to crash if the page is used in way disallowed by HW 
(i.e. for executable code or as a page table).

Normally set_memory() call failures are rare. But on TDX 
set_memory_XXcrypted() involves calls to the untrusted VMM, and an attacker
could fail these calls such that:
 1. set_memory_encrypted() returns an error and leaves the pages fully
    shared.
 2. set_memory_decrypted() returns an error, but the pages are actually
    full converted to shared.

This means that patterns like the below can cause problems:
void *addr = alloc();
int fail = set_memory_decrypted(addr, 1);
if (fail)
	free_pages(addr, 0);

And:
void *addr = alloc();
int fail = set_memory_decrypted(addr, 1);
if (fail) {
	set_memory_encrypted(addr, 1);
	free_pages(addr, 0);
}

Unfortunately these patterns are all over the place. And what the 
set_memory() callers should do in this situation is not clear either. They 
shouldn’t use them as shared because something clearly went wrong, but 
they also need to fully reset the pages to private to free them. But, the 
kernel needs the VMMs help to do this and the VMM is already being 
uncooperative around the needed operations. So this isn't guaranteed to 
succeed and the caller is kind of stuck with unusable pages.

Looking at QEMU/KVM as an example, these VMM converstion failures either 
indicates an attempt to attack the guest, or resource constraints on the 
host. Preventing a DOS attack is out of scope for the coco threat model. 
So this leaves the host resource constraint cause. When similar resource 
constraints are encountered in the host, KVM punts the problem to 
userspace and QEMU terminates the guest. When similar problems are 
detected inside set_memory(), SEV issues a command to terminate the guest. 

This all makes it appealing to simply panic (via tdx_panic() call 
which informs the host what is happening) when observing troublesome VMM 
behavior around the memory conversion. It is:
 - Consistent with similar behavior on SEV side.
 - Generally more consistent with how host resource constraints are handled
   (at least in QEMU/KVM)
 - Would be a more foolproof defense against the attack scenario.

Never-the-less, doing so would be an instance of the “crash the kernel for 
security reasons” pattern. This is a big reason, and crashing is not fully 
needed because the unusable pages could just be leaked (as they already 
are in some cases). So instead, this series does a tree-wide search and 
fixes the callers to handle the error by leaking the pages. Going forward 
callers will need to handle the set_memory() errors correctly in order to 
not reintroduce the issue.

I think there are some points for both sides, and we had some internal
discussion on the right way to handle it. So I've tried to characterize
both arguments. I'm interested to hear opinions on which is the best.

I’ve marked the hyperv guest parts in this as RFC, both because I can’t 
test them and I believe Linux TDs can’t run on hyperv yet due to some
missing support. I would appreciate a correction on this if it’s wrong.

Rick Edgecombe (10):
  mm: Add helper for freeing decrypted memory
  x86/mm/cpa: Reject incorrect encryption change requests
  kvmclock: Use free_decrypted_pages()
  swiotlb: Use free_decrypted_pages()
  ptp: Use free_decrypted_pages()
  dma: Use free_decrypted_pages()
  hv: Use free_decrypted_pages()
  hv: Track decrypted status in vmbus_gpadl
  hv_nstvsc: Don't free decrypted memory
  uio_hv_generic: Don't free decrypted memory

 arch/s390/include/asm/set_memory.h |  1 +
 arch/x86/kernel/kvmclock.c         |  2 +-
 arch/x86/mm/pat/set_memory.c       | 41 +++++++++++++++++++++++++++++-
 drivers/hv/channel.c               | 18 ++++++++-----
 drivers/hv/connection.c            | 13 +++++++---
 drivers/net/hyperv/netvsc.c        |  7 +++--
 drivers/ptp/ptp_kvm_x86.c          |  2 +-
 drivers/uio/uio_hv_generic.c       | 12 ++++++---
 include/linux/dma-map-ops.h        |  3 ++-
 include/linux/hyperv.h             |  1 +
 include/linux/set_memory.h         | 13 ++++++++++
 kernel/dma/contiguous.c            |  2 +-
 kernel/dma/swiotlb.c               | 11 +++++---
 13 files changed, 101 insertions(+), 25 deletions(-)

-- 
2.34.1

Powered by blists - more mailing lists