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

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

[Sean Christopherson]

Add compile-time and init-time sanity checks to ensure that the MMIO SPTE
mask doesn't overlap the MMIO SPTE generation.  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 MMIO
bits doesn't silently break MMIO caching.

Suggested-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/spte.c | 8 ++++++++
 arch/x86/kvm/mmu/spte.h | 9 +++++++++
 2 files changed, 17 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..6cd3936cbe1f 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -125,6 +125,15 @@ 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 is allowed to use "present" bits (i.e. all EPT RWX bits),
+ * all physical address bits (additional checks ensure the mask doesn't overlap
+ * legal PA bits), and bit 63 (carved out for future usage, e.g. SUPPRESS_VE).
+ */
+#define SPTE_MMIO_ALLOWED_MASK (BIT_ULL(63) | GENMASK_ULL(51, 12) | GENMASK_ULL(2, 0))
+static_assert(!(SPTE_MMIO_ALLOWED_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] KVM: x86/mmu: Add sanity check that MMIO SPTE mask doesn't overlap gen

[Kai Huang]

On Wed, 2022-08-03 at 21:33 +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.  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 MMIO
> bits doesn't silently break MMIO caching.

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

Btw, should you also check SPTE_MMU_PRESENT_MASK (or in another patch)?

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

Thanks for giving me the credit as I don't feel I deserve it.

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu/spte.c | 8 ++++++++
>  arch/x86/kvm/mmu/spte.h | 9 +++++++++
>  2 files changed, 17 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..6cd3936cbe1f 100644
> --- a/arch/x86/kvm/mmu/spte.h
> +++ b/arch/x86/kvm/mmu/spte.h
> @@ -125,6 +125,15 @@ 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 is allowed to use "present" bits (i.e. all EPT RWX bits),
> + * all physical address bits (additional checks ensure the mask doesn't overlap
> + * legal PA bits), and bit 63 (carved out for future usage, e.g. SUPPRESS_VE).
> + */
> +#define SPTE_MMIO_ALLOWED_MASK (BIT_ULL(63) | GENMASK_ULL(51, 12) | GENMASK_ULL(2, 0))
> +static_assert(!(SPTE_MMIO_ALLOWED_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

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

[Sean Christopherson]

On Thu, Aug 04, 2022, Kai Huang wrote:
> On Wed, 2022-08-03 at 21:33 +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.  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 MMIO
> > bits doesn't silently break MMIO caching.
> 
> Reviwed-by: Kai Huang <kai.huang@intel.com>
> 
> Btw, should you also check SPTE_MMU_PRESENT_MASK (or in another patch)?

Rats, I thought we already checked that, but it's only the MMIO generation that
checks for overlap.  I'll send a v2.