[PATCH v2] KVM: x86/mmu: Add sanity check that MMIO SPTE mask doesn't overlap gen

Sean Christopherson posted 1 patch 6 days, 3 hours ago
arch/x86/kvm/mmu/spte.c |  8 ++++++++
arch/x86/kvm/mmu/spte.h | 14 ++++++++++++++
2 files changed, 22 insertions(+)
[PATCH v2] KVM: x86/mmu: Add sanity check that MMIO SPTE mask doesn't overlap gen
Posted by Sean Christopherson 6 days, 3 hours ago
Add compile-time and init-time sanity checks to ensure that the MMIO SPTE
mask doesn't overlap the MMIO SPTE generation or the MMU-present bit.
The generation currently avoids using bit 63, but that's as much
coincidence as it is strictly necessarly.  That will change in the future,
as TDX support will require setting bit 63 (SUPPRESS_VE) in the mask.

Explicitly carve out the bits that are allowed in the mask so that any
future shuffling of SPTE bits doesn't silently break MMIO caching (KVM
has broken MMIO caching more than once due to overlapping the generation
with other things).

Suggested-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

Kai, I didn't included your review since I pretty much rewrote the entire
comment.

v2: Prevent overlap with SPTE_MMU_PRESENT_MASK
v1: https://lore.kernel.org/all/20220803213354.951376-1-seanjc@google.com

 arch/x86/kvm/mmu/spte.c |  8 ++++++++
 arch/x86/kvm/mmu/spte.h | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 7314d27d57a4..08e8c46f3037 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -343,6 +343,14 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask)
 	if (!enable_mmio_caching)
 		mmio_value = 0;
 
+	/*
+	 * The mask must contain only bits that are carved out specifically for
+	 * the MMIO SPTE mask, e.g. to ensure there's no overlap with the MMIO
+	 * generation.
+	 */
+	if (WARN_ON(mmio_mask & ~SPTE_MMIO_ALLOWED_MASK))
+		mmio_value = 0;
+
 	/*
 	 * Disable MMIO caching if the MMIO value collides with the bits that
 	 * are used to hold the relocated GFN when the L1TF mitigation is
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index cabe3fbb4f39..10f16421e876 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -125,6 +125,20 @@ static_assert(!(EPT_SPTE_MMU_WRITABLE & SHADOW_ACC_TRACK_SAVED_MASK));
 static_assert(!(SPTE_MMU_PRESENT_MASK &
 		(MMIO_SPTE_GEN_LOW_MASK | MMIO_SPTE_GEN_HIGH_MASK)));
 
+/*
+ * The SPTE MMIO mask must NOT overlap the MMIO generation bits or the
+ * MMU-present bit.  The generation obviously co-exists with the magic MMIO
+ * mask/value, and MMIO SPTEs are considered !MMU-present.
+ *
+ * The SPTE MMIO mask is allowed to use hardware "present" bits (i.e. all EPT
+ * RWX bits), all physical address bits (legal PA bits are used for "fast" MMIO
+ * and so they're off-limits for generation; additional checks ensure the mask
+ * doesn't overlap legal PA bits), and bit 63 (carved out for future usage).
+ */
+#define SPTE_MMIO_ALLOWED_MASK (BIT_ULL(63) | GENMASK_ULL(51, 12) | GENMASK_ULL(2, 0))
+static_assert(!(SPTE_MMIO_ALLOWED_MASK &
+		(SPTE_MMU_PRESENT_MASK | MMIO_SPTE_GEN_LOW_MASK | MMIO_SPTE_GEN_HIGH_MASK)));
+
 #define MMIO_SPTE_GEN_LOW_BITS		(MMIO_SPTE_GEN_LOW_END - MMIO_SPTE_GEN_LOW_START + 1)
 #define MMIO_SPTE_GEN_HIGH_BITS		(MMIO_SPTE_GEN_HIGH_END - MMIO_SPTE_GEN_HIGH_START + 1)
 

base-commit: 93472b79715378a2386598d6632c654a2223267b
-- 
2.37.1.559.g78731f0fdb-goog
Re: [PATCH v2] KVM: x86/mmu: Add sanity check that MMIO SPTE mask doesn't overlap gen
Posted by Paolo Bonzini 2 days, 8 hours ago
Queued, thanks.

Paolo
Re: [PATCH v2] KVM: x86/mmu: Add sanity check that MMIO SPTE mask doesn't overlap gen
Posted by Huang, Kai 5 days, 22 hours ago
On Fri, 2022-08-05 at 19:41 +0000, Sean Christopherson wrote:
> Add compile-time and init-time sanity checks to ensure that the MMIO SPTE
> mask doesn't overlap the MMIO SPTE generation or the MMU-present bit.
> The generation currently avoids using bit 63, but that's as much
> coincidence as it is strictly necessarly.  That will change in the future,
> as TDX support will require setting bit 63 (SUPPRESS_VE) in the mask.
> 
> Explicitly carve out the bits that are allowed in the mask so that any
> future shuffling of SPTE bits doesn't silently break MMIO caching (KVM
> has broken MMIO caching more than once due to overlapping the generation
> with other things).
> 
> Suggested-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> Kai, I didn't included your review since I pretty much rewrote the entire
> comment.
> 
> v2: Prevent overlap with SPTE_MMU_PRESENT_MASK
> v1: https://lore.kernel.org/all/20220803213354.951376-1-seanjc@google.com

Reviewed-by: Kai Huang <kai.huang@intel.com>


-- 
Thanks,
-Kai