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@xxxxxxxxx>

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

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

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

> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  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