Date:   Sat, 04 Jun 2022 09:26:13 +0100
Message-ID: <87v8tgltqy.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Peter Collingbourne <pcc@google.com>
Cc:     Fuad Tabba <tabba@google.com>, Will Deacon <will@kernel.org>,
        kvmarm@lists.cs.columbia.edu, Ard Biesheuvel <ardb@kernel.org>,
        Sean Christopherson <seanjc@google.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Andy Lutomirski <luto@amacapital.net>,
        Catalin Marinas <catalin.marinas@arm.com>,
        James Morse <james.morse@arm.com>,
        Chao Peng <chao.p.peng@linux.intel.com>,
        Quentin Perret <qperret@google.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Michael Roth <michael.roth@amd.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Oliver Upton <oupton@google.com>, kernel-team@android.com,
        kvm@vger.kernel.org,
        Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 59/89] KVM: arm64: Do not support MTE for protected VMs
In-Reply-To: <Ypl5TdMN3J/tttNe@google.com>
References: <20220519134204.5379-1-will@kernel.org>
        <20220519134204.5379-60-will@kernel.org>
        <CAMn1gO4_d75_88fg5hcnBqx+tdu-9pG7atzt-qUD1nhUNs5TyQ@mail.gmail.com>
        <CA+EHjTx328na4FDfKU-cdLX+SV4MmKfMKKrTHo5H0=iB2GTQ+A@mail.gmail.com>
        <Ypl5TdMN3J/tttNe@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 03 Jun 2022 04:00:29 +0100,
Peter Collingbourne <pcc@google.com> wrote:
>=20
> Hi Fuad,
>=20
> On Fri, May 27, 2022 at 08:55:42AM +0100, Fuad Tabba wrote:
> > Hi Peter,
> >=20
> > On Thu, May 26, 2022 at 9:08 PM Peter Collingbourne <pcc@google.com> wr=
ote:
> > >
> > > On Thu, May 19, 2022 at 7:40 AM Will Deacon <will@kernel.org> wrote:
> > > >
> > > > From: Fuad Tabba <tabba@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.
> >=20
> > Yes, we need to explain this more. Basically this is an extension of
> > restricting features for protected VMs done earlier [*].
> >=20
> > Various VM feature configurations are allowed in KVM/arm64, each requir=
ing
> > specific handling logic to deal with traps, context-switching and poten=
tially
> > emulation. Achieving feature parity in pKVM therefore requires either e=
levating
> > 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.
> >=20
> > 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.
> >=20
> > Also note that this only applies to protected VMs. Non-protected VMs
> > in protected mode support MTE.
>=20
> I see. In this case unless I'm missing something the=C2=A0EL2 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).
>=20
> [1] https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm=
/+/3689015
>=20
> From c87965cd14515586d487872486e7670874209113 Mon Sep 17 00:00:00 2001
> From: Peter Collingbourne <pcc@google.com>
> Date: Thu, 2 Jun 2022 19:16:02 -0700
> Subject: [PATCH] arm64: support MTE in protected VMs
>=20
> Enable HCR_EL2.ATA while running a vCPU with MTE enabled.
>=20
> To avoid exposing MTE tags from the host to protected VMs, sanitize
> tags before donating pages.
>=20
> Signed-off-by: Peter Collingbourne <pcc@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(-)
>=20
> diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/k=
vm_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) \
>  	)
> =20
>  /*
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkv=
m.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 *vc=
pu)
>  	/* Memory Tagging: Trap and Treat as Untagged if not supported. */
>  	if (!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR1_MTE), feature_ids)) {
>  		hcr_set |=3D HCR_TID5;
> -		hcr_clear |=3D HCR_DCT | HCR_ATA;
> +		hcr_clear |=3D HCR_ATA;
>  	}
> =20
>  	vcpu->arch.hcr_el2 |=3D 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 =3D HCR_GUEST_FLAGS |
> -			     HCR_TID3 | HCR_TACR | HCR_TIDCP | HCR_TID1;
> +	vcpu->arch.hcr_el2 =3D HCR_GUEST_FLAGS | HCR_TID3 | HCR_TACR | HCR_TIDC=
P |
> +			     HCR_TID1 | HCR_ATA;
> =20
>  	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, p=
hys_addr_t fault_ipa,
>  		goto dec_account;
>  	}
> =20
> -	write_lock(&kvm->mmu_lock);
>  	pfn =3D 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 =3D pkvm_host_map_guest(pfn, fault_ipa >> PAGE_SHIFT);
>  	if (ret) {
>  		if (ret =3D=3D -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.

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