[PATCH 59/89] KVM: arm64: Do not support MTE for protected VMs

[Marc Zyngier]

On Fri, 03 Jun 2022 04:00:29 +0100,
Peter Collingbourne <pcc at google.com> wrote:
> 
> Hi Fuad,
> 
> On Fri, May 27, 2022 at 08:55:42AM +0100, Fuad Tabba wrote:
> > Hi Peter,
> > 
> > On Thu, May 26, 2022 at 9:08 PM Peter Collingbourne <pcc at google.com> wrote:
> > >
> > > On Thu, May 19, 2022 at 7:40 AM Will Deacon <will at kernel.org> wrote:
> > > >
> > > > From: Fuad Tabba <tabba at google.com>
> > > >
> > > > Return an error (-EINVAL) if trying to enable MTE on a protected
> > > > vm.
> > >
> > > I think this commit message needs more explanation as to why MTE is
> > > not currently supported in protected VMs.
> > 
> > Yes, we need to explain this more. Basically this is an extension of
> > restricting features for protected VMs done earlier [*].
> > 
> > Various VM feature configurations are allowed in KVM/arm64, each requiring
> > specific handling logic to deal with traps, context-switching and potentially
> > emulation. Achieving feature parity in pKVM therefore requires either elevating
> > this logic to EL2 (and substantially increasing the TCB) or continuing to trust
> > the host handlers at EL1. Since neither of these options are especially
> > appealing, pKVM instead limits the CPU features exposed to a guest to a fixed
> > configuration based on the underlying hardware and which can mostly be provided
> > straightforwardly by EL2.
> > 
> > This of course can change in the future and we can support more
> > features for protected VMs as needed. We'll expand on this commit
> > message when we respin.
> > 
> > Also note that this only applies to protected VMs. Non-protected VMs
> > in protected mode support MTE.
> 
> I see. In this case unless I'm missing something the EL2 side seems
> quite trivial though (flipping some bits in HCR_EL2). The patch below
> (in place of this one) seems to make MTE work in my test environment
> (patched [1] crosvm on Android in MTE-enabled QEMU).
> 
> [1] https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3689015
> 
> From c87965cd14515586d487872486e7670874209113 Mon Sep 17 00:00:00 2001
> From: Peter Collingbourne <pcc at google.com>
> Date: Thu, 2 Jun 2022 19:16:02 -0700
> Subject: [PATCH] arm64: support MTE in protected VMs
> 
> Enable HCR_EL2.ATA while running a vCPU with MTE enabled.
> 
> To avoid exposing MTE tags from the host to protected VMs, sanitize
> tags before donating pages.
> 
> Signed-off-by: Peter Collingbourne <pcc at google.com>
> ---
>  arch/arm64/include/asm/kvm_pkvm.h | 4 +++-
>  arch/arm64/kvm/hyp/nvhe/pkvm.c    | 6 +++---
>  arch/arm64/kvm/mmu.c              | 4 +++-
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
> index 952e3c3fa32d..9ca9296f2a25 100644
> --- a/arch/arm64/include/asm/kvm_pkvm.h
> +++ b/arch/arm64/include/asm/kvm_pkvm.h
> @@ -73,10 +73,12 @@ void kvm_shadow_destroy(struct kvm *kvm);
>   * Allow for protected VMs:
>   * - Branch Target Identification
>   * - Speculative Store Bypassing
> + * - Memory Tagging Extension
>   */
>  #define PVM_ID_AA64PFR1_ALLOW (\
>  	ARM64_FEATURE_MASK(ID_AA64PFR1_BT) | \
> -	ARM64_FEATURE_MASK(ID_AA64PFR1_SSBS) \
> +	ARM64_FEATURE_MASK(ID_AA64PFR1_SSBS) | \
> +	ARM64_FEATURE_MASK(ID_AA64PFR1_MTE) \
>  	)
>  
>  /*
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index e33ba9067d7b..46ddd9093ac7 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -88,7 +88,7 @@ static void pvm_init_traps_aa64pfr1(struct kvm_vcpu *vcpu)
>  	/* Memory Tagging: Trap and Treat as Untagged if not supported. */
>  	if (!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR1_MTE), feature_ids)) {
>  		hcr_set |= HCR_TID5;
> -		hcr_clear |= HCR_DCT | HCR_ATA;
> +		hcr_clear |= HCR_ATA;
>  	}
>  
>  	vcpu->arch.hcr_el2 |= hcr_set;
> @@ -179,8 +179,8 @@ static void pvm_init_trap_regs(struct kvm_vcpu *vcpu)
>  	 * - Feature id registers: to control features exposed to guests
>  	 * - Implementation-defined features
>  	 */
> -	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS |
> -			     HCR_TID3 | HCR_TACR | HCR_TIDCP | HCR_TID1;
> +	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS | HCR_TID3 | HCR_TACR | HCR_TIDCP |
> +			     HCR_TID1 | HCR_ATA;
>  
>  	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN)) {
>  		/* route synchronous external abort exceptions to EL2 */
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 392ff7b2362d..f513852357f7 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1206,8 +1206,10 @@ static int pkvm_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		goto dec_account;
>  	}
>  
> -	write_lock(&kvm->mmu_lock);
>  	pfn = page_to_pfn(page);
> +	sanitise_mte_tags(kvm, pfn, PAGE_SIZE);
> +
> +	write_lock(&kvm->mmu_lock);

Is it really safe to rely on the host to clear the tags? My guts
feeling says that it isn't. If it is required, we cannot leave this
responsibility to the host, and this logic must be moved to EL2. And
if it isn't, then we should drop it.

>  	ret = pkvm_host_map_guest(pfn, fault_ipa >> PAGE_SHIFT);
>  	if (ret) {
>  		if (ret == -EAGAIN)

But the bigger picture here is what ensures that the host cannot mess
with the guest tags? I don't think we have a any mechanism to
guarantee that, specially on systems where the tags are only a memory
carve-out, which the host could map and change at will.

In any case, this isn't the time to pile new features on top of
pKVM. The current plan is to not support MTE at all, and only do it
once we have a definitive story on page donation (which as you may
have noticed, is pretty hacky). I don't see any compelling reason to
add MTE to the mix until this is solved.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.