[PATCH v4] arm64: mte: move register initialization to C

[PATCH v4] arm64: mte: move register initialization to C

[Evgenii Stepanov]

If FEAT_MTE2 is disabled via the arm64.nomte command line argument on a
CPU that claims to support FEAT_MTE2, the kernel will use Tagged Normal
in the MAIR. If we interpret arm64.nomte to mean that the CPU does not
in fact implement FEAT_MTE2, setting the system register like this may
lead to UNSPECIFIED behavior. Fix it by arranging for MAIR to be set
in the C function cpu_enable_mte which is called based on the sanitized
version of the system register.

There is no need for the rest of the MTE-related system register
initialization to happen from assembly, with the exception of TCR_EL1,
which must be set to include at least TBI1 because the secondary CPUs
access KASan-allocated data structures early. Therefore, make the TCR_EL1
initialization unconditional and move the rest of the initialization to
cpu_enable_mte so that we no longer have a dependency on the unsanitized
ID register value.

Signed-off-by: Peter Collingbourne <pcc@google.com>
Signed-off-by: Evgenii Stepanov <eugenis@google.com>
Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: kernel test robot <lkp@intel.com>
---
Changelong since v3:
- Removed extra isb barrier.
- Fixed build without CONFIG_ARM64_MTE.

Changelog since v2:
- Fixed register initialization on cpu_resume code path.

Changelog since v1:
- Keep TBI1 off unless CONFIG_ARM64_MTE
- Fixed mask application in the RGSR_EL1 computation (bug found by Kenny
  Root).
- Changed code formatting

 arch/arm64/include/asm/mte.h   |  5 ++++
 arch/arm64/kernel/cpufeature.c |  3 +-
 arch/arm64/kernel/mte.c        | 51 ++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/suspend.c    |  2 ++
 arch/arm64/mm/proc.S           | 46 ++++--------------------------
 5 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index aa523591a44e5..760c62f8e22f8 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -42,7 +42,9 @@ void mte_sync_tags(pte_t old_pte, pte_t pte);
 void mte_copy_page_tags(void *kto, const void *kfrom);
 void mte_thread_init_user(void);
 void mte_thread_switch(struct task_struct *next);
+void mte_cpu_setup(void);
 void mte_suspend_enter(void);
+void mte_suspend_exit(void);
 long set_mte_ctrl(struct task_struct *task, unsigned long arg);
 long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
@@ -72,6 +74,9 @@ static inline void mte_thread_switch(struct task_struct *next)
 static inline void mte_suspend_enter(void)
 {
 }
+static inline void mte_suspend_exit(void)
+{
+}
 static inline long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
 	return 0;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index af4de817d7123..d7a077b5ccd1c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2034,7 +2034,8 @@ static void bti_enable(const struct arm64_cpu_capabilities *__unused)
 static void cpu_enable_mte(struct arm64_cpu_capabilities const *cap)
 {
 	sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ATA | SCTLR_EL1_ATA0);
-	isb();
+
+	mte_cpu_setup();
 
 	/*
 	 * Clear the tags in the zero page. This needs to be done via the
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index b2b730233274b..aca88470fb69d 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -285,6 +285,49 @@ void mte_thread_switch(struct task_struct *next)
 	mte_check_tfsr_el1();
 }
 
+void mte_cpu_setup(void)
+{
+	u64 rgsr;
+
+	/*
+	 * CnP must be enabled only after the MAIR_EL1 register has been set
+	 * up. Inconsistent MAIR_EL1 between CPUs sharing the same TLB may
+	 * lead to the wrong memory type being used for a brief window during
+	 * CPU power-up.
+	 *
+	 * CnP is not a boot feature so MTE gets enabled before CnP, but let's
+	 * make sure that is the case.
+	 */
+	BUG_ON(read_sysreg(ttbr0_el1) & TTBR_CNP_BIT);
+	BUG_ON(read_sysreg(ttbr1_el1) & TTBR_CNP_BIT);
+
+	/* Normal Tagged memory type at the corresponding MAIR index */
+	sysreg_clear_set(mair_el1,
+			 MAIR_ATTRIDX(MAIR_ATTR_MASK, MT_NORMAL_TAGGED),
+			 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_TAGGED,
+				      MT_NORMAL_TAGGED));
+
+	write_sysreg_s(KERNEL_GCR_EL1, SYS_GCR_EL1);
+
+	/*
+	 * If GCR_EL1.RRND=1 is implemented the same way as RRND=0, then
+	 * RGSR_EL1.SEED must be non-zero for IRG to produce
+	 * pseudorandom numbers. As RGSR_EL1 is UNKNOWN out of reset, we
+	 * must initialize it.
+	 */
+	rgsr = (read_sysreg(CNTVCT_EL0) & SYS_RGSR_EL1_SEED_MASK) <<
+	       SYS_RGSR_EL1_SEED_SHIFT;
+	if (rgsr == 0)
+		rgsr = 1 << SYS_RGSR_EL1_SEED_SHIFT;
+	write_sysreg_s(rgsr, SYS_RGSR_EL1);
+
+	/* clear any pending tag check faults in TFSR*_EL1 */
+	write_sysreg_s(0, SYS_TFSR_EL1);
+	write_sysreg_s(0, SYS_TFSRE0_EL1);
+
+	local_flush_tlb_all();
+}
+
 void mte_suspend_enter(void)
 {
 	if (!system_supports_mte())
@@ -301,6 +344,14 @@ void mte_suspend_enter(void)
 	mte_check_tfsr_el1();
 }
 
+void mte_suspend_exit(void)
+{
+	if (!system_supports_mte())
+		return;
+
+	mte_cpu_setup();
+}
+
 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
 	u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index 9135fe0f3df53..8b02d310838f9 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -43,6 +43,8 @@ void notrace __cpu_suspend_exit(void)
 {
 	unsigned int cpu = smp_processor_id();
 
+	mte_suspend_exit();
+
 	/*
 	 * We are resuming from reset with the idmap active in TTBR0_EL1.
 	 * We must uninstall the idmap and restore the expected MMU
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 7837a69524c53..f38bccdd374a5 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -48,17 +48,19 @@
 
 #ifdef CONFIG_KASAN_HW_TAGS
 #define TCR_MTE_FLAGS TCR_TCMA1 | TCR_TBI1 | TCR_TBID1
-#else
+#elif defined(CONFIG_ARM64_MTE)
 /*
  * The mte_zero_clear_page_tags() implementation uses DC GZVA, which relies on
  * TBI being enabled at EL1.
  */
 #define TCR_MTE_FLAGS TCR_TBI1 | TCR_TBID1
+#else
+#define TCR_MTE_FLAGS 0
 #endif
 
 /*
  * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
- * changed during __cpu_setup to Normal Tagged if the system supports MTE.
+ * changed during mte_cpu_setup to Normal Tagged if the system supports MTE.
  */
 #define MAIR_EL1_SET							\
 	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
@@ -426,46 +428,8 @@ SYM_FUNC_START(__cpu_setup)
 	mov_q	mair, MAIR_EL1_SET
 	mov_q	tcr, TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | \
 			TCR_TG_FLAGS | TCR_KASLR_FLAGS | TCR_ASID16 | \
-			TCR_TBI0 | TCR_A1 | TCR_KASAN_SW_FLAGS
-
-#ifdef CONFIG_ARM64_MTE
-	/*
-	 * Update MAIR_EL1, GCR_EL1 and TFSR*_EL1 if MTE is supported
-	 * (ID_AA64PFR1_EL1[11:8] > 1).
-	 */
-	mrs	x10, ID_AA64PFR1_EL1
-	ubfx	x10, x10, #ID_AA64PFR1_MTE_SHIFT, #4
-	cmp	x10, #ID_AA64PFR1_MTE
-	b.lt	1f
-
-	/* Normal Tagged memory type at the corresponding MAIR index */
-	mov	x10, #MAIR_ATTR_NORMAL_TAGGED
-	bfi	mair, x10, #(8 *  MT_NORMAL_TAGGED), #8
+			TCR_TBI0 | TCR_A1 | TCR_KASAN_SW_FLAGS | TCR_MTE_FLAGS
 
-	mov	x10, #KERNEL_GCR_EL1
-	msr_s	SYS_GCR_EL1, x10
-
-	/*
-	 * If GCR_EL1.RRND=1 is implemented the same way as RRND=0, then
-	 * RGSR_EL1.SEED must be non-zero for IRG to produce
-	 * pseudorandom numbers. As RGSR_EL1 is UNKNOWN out of reset, we
-	 * must initialize it.
-	 */
-	mrs	x10, CNTVCT_EL0
-	ands	x10, x10, #SYS_RGSR_EL1_SEED_MASK
-	csinc	x10, x10, xzr, ne
-	lsl	x10, x10, #SYS_RGSR_EL1_SEED_SHIFT
-	msr_s	SYS_RGSR_EL1, x10
-
-	/* clear any pending tag check faults in TFSR*_EL1 */
-	msr_s	SYS_TFSR_EL1, xzr
-	msr_s	SYS_TFSRE0_EL1, xzr
-
-	/* set the TCR_EL1 bits */
-	mov_q	x10, TCR_MTE_FLAGS
-	orr	tcr, tcr, x10
-1:
-#endif
 	tcr_clear_errata_bits tcr, x9, x5
 
 #ifdef CONFIG_ARM64_VA_BITS_52
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] arm64: mte: move register initialization to C

[Peter Collingbourne]

On Tue, Sep 6, 2022 at 5:36 PM Evgenii Stepanov <eugenis@google.com> wrote:
>
> If FEAT_MTE2 is disabled via the arm64.nomte command line argument on a
> CPU that claims to support FEAT_MTE2, the kernel will use Tagged Normal
> in the MAIR. If we interpret arm64.nomte to mean that the CPU does not
> in fact implement FEAT_MTE2, setting the system register like this may
> lead to UNSPECIFIED behavior. Fix it by arranging for MAIR to be set
> in the C function cpu_enable_mte which is called based on the sanitized
> version of the system register.
>
> There is no need for the rest of the MTE-related system register
> initialization to happen from assembly, with the exception of TCR_EL1,
> which must be set to include at least TBI1 because the secondary CPUs
> access KASan-allocated data structures early. Therefore, make the TCR_EL1
> initialization unconditional and move the rest of the initialization to
> cpu_enable_mte so that we no longer have a dependency on the unsanitized
> ID register value.

Moving the register initialization to C also fixes a bug where the
kernel's zeroing of TFSR_EL1 has no practical effect when the kernel
is started in VHE mode because the register is currently being zeroed
prior to the kernel enabling the redirect of TFSR_EL2 to TFSR_EL1 when
it enables VHE. As a result, without this patch it is possible to get
a spurious KASAN error report if TFSR_EL2 is non-zero out of reset.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] arm64: mte: move register initialization to C

[Catalin Marinas]

On Fri, Sep 09, 2022 at 05:54:13PM -0700, Peter Collingbourne wrote:
> On Tue, Sep 6, 2022 at 5:36 PM Evgenii Stepanov <eugenis@google.com> wrote:
> > If FEAT_MTE2 is disabled via the arm64.nomte command line argument on a
> > CPU that claims to support FEAT_MTE2, the kernel will use Tagged Normal
> > in the MAIR. If we interpret arm64.nomte to mean that the CPU does not
> > in fact implement FEAT_MTE2, setting the system register like this may
> > lead to UNSPECIFIED behavior. Fix it by arranging for MAIR to be set
> > in the C function cpu_enable_mte which is called based on the sanitized
> > version of the system register.
> >
> > There is no need for the rest of the MTE-related system register
> > initialization to happen from assembly, with the exception of TCR_EL1,
> > which must be set to include at least TBI1 because the secondary CPUs
> > access KASan-allocated data structures early. Therefore, make the TCR_EL1
> > initialization unconditional and move the rest of the initialization to
> > cpu_enable_mte so that we no longer have a dependency on the unsanitized
> > ID register value.
> 
> Moving the register initialization to C also fixes a bug where the
> kernel's zeroing of TFSR_EL1 has no practical effect when the kernel
> is started in VHE mode because the register is currently being zeroed
> prior to the kernel enabling the redirect of TFSR_EL2 to TFSR_EL1 when
> it enables VHE. As a result, without this patch it is possible to get
> a spurious KASAN error report if TFSR_EL2 is non-zero out of reset.

Oh, I think this is a side-effect of the nVHE patches. We added MTE in
5.10 and __cpu_setup() was called at EL2 if the kernel was entered at
EL2 - 3b714d24ef17 ("arm64: mte: CPU feature detection and initial
sysreg configuration"). When nVHE turned up in 5.12, this was changed to
to run __cpu_setup at EL1 and this only initialises TFSR_EL1.
__finalise_el2 should have transferred TFSR_EL12.

I don't think there other registers we missed in __cpu_setup() but I
haven't looked in detail.

So for this, we either move the reg initialisation to C or we fix
__finalise_el2. I'm tempted to go with the former as long as the kernel
doesn't read that register up to that point and complain of a spurious
asynchronous fault.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] arm64: mte: move register initialization to C

[Peter Collingbourne]

On Mon, Sep 12, 2022 at 1:04 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Sep 09, 2022 at 05:54:13PM -0700, Peter Collingbourne wrote:
> > On Tue, Sep 6, 2022 at 5:36 PM Evgenii Stepanov <eugenis@google.com> wrote:
> > > If FEAT_MTE2 is disabled via the arm64.nomte command line argument on a
> > > CPU that claims to support FEAT_MTE2, the kernel will use Tagged Normal
> > > in the MAIR. If we interpret arm64.nomte to mean that the CPU does not
> > > in fact implement FEAT_MTE2, setting the system register like this may
> > > lead to UNSPECIFIED behavior. Fix it by arranging for MAIR to be set
> > > in the C function cpu_enable_mte which is called based on the sanitized
> > > version of the system register.
> > >
> > > There is no need for the rest of the MTE-related system register
> > > initialization to happen from assembly, with the exception of TCR_EL1,
> > > which must be set to include at least TBI1 because the secondary CPUs
> > > access KASan-allocated data structures early. Therefore, make the TCR_EL1
> > > initialization unconditional and move the rest of the initialization to
> > > cpu_enable_mte so that we no longer have a dependency on the unsanitized
> > > ID register value.
> >
> > Moving the register initialization to C also fixes a bug where the
> > kernel's zeroing of TFSR_EL1 has no practical effect when the kernel
> > is started in VHE mode because the register is currently being zeroed
> > prior to the kernel enabling the redirect of TFSR_EL2 to TFSR_EL1 when
> > it enables VHE. As a result, without this patch it is possible to get
> > a spurious KASAN error report if TFSR_EL2 is non-zero out of reset.
>
> Oh, I think this is a side-effect of the nVHE patches. We added MTE in
> 5.10 and __cpu_setup() was called at EL2 if the kernel was entered at
> EL2 - 3b714d24ef17 ("arm64: mte: CPU feature detection and initial
> sysreg configuration"). When nVHE turned up in 5.12, this was changed to
> to run __cpu_setup at EL1 and this only initialises TFSR_EL1.
> __finalise_el2 should have transferred TFSR_EL12.
>
> I don't think there other registers we missed in __cpu_setup() but I
> haven't looked in detail.
>
> So for this, we either move the reg initialisation to C or we fix
> __finalise_el2. I'm tempted to go with the former as long as the kernel
> doesn't read that register up to that point and complain of a spurious
> asynchronous fault.

Yes, I don't think we can generally expect the kernel to read the
register up to that point. The register is generally only read in
response to an exception, and the early kernel initialization code
runs with interrupts disabled and only enables them well after the
call to smp_prepare_boot_cpu() for the primary CPU (see the calls to
local_irq_disable() and local_irq_enable() in start_kernel()) and
check_local_cpu_capabilities() for the secondaries (see the call to
local_daif_restore() in secondary_start_kernel()), so unless something
has gone seriously wrong that would cause us to get a synchronous
exception that early (in which case the spurious async fault report is
the least of our worries) I don't think we should expect to see one of
these reports as a result of moving the initialization code to C.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] arm64: mte: move register initialization to C

[Catalin Marinas]

On Tue, Sep 06, 2022 at 05:36:30PM -0700, Evgenii Stepanov wrote:
> If FEAT_MTE2 is disabled via the arm64.nomte command line argument on a
> CPU that claims to support FEAT_MTE2, the kernel will use Tagged Normal
> in the MAIR. If we interpret arm64.nomte to mean that the CPU does not
> in fact implement FEAT_MTE2, setting the system register like this may
> lead to UNSPECIFIED behavior. Fix it by arranging for MAIR to be set
> in the C function cpu_enable_mte which is called based on the sanitized
> version of the system register.
> 
> There is no need for the rest of the MTE-related system register
> initialization to happen from assembly, with the exception of TCR_EL1,
> which must be set to include at least TBI1 because the secondary CPUs
> access KASan-allocated data structures early. Therefore, make the TCR_EL1
> initialization unconditional and move the rest of the initialization to
> cpu_enable_mte so that we no longer have a dependency on the unsanitized
> ID register value.
> 
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Signed-off-by: Evgenii Stepanov <eugenis@google.com>

Does this need a Co-developed-by from Peter? The sign-off on its own
doesn't make sense unless Peter sent the patch.

If we want this as a fix (though we have an alternative to transfer
mair_el12 to mair_el2), I'd also add:

Fixes: 3b714d24ef17 ("arm64: mte: CPU feature detection and initial sysreg configuration")
Cc: <stable@vger.kernel.org> # 5.10.x

together with some comment what it fixes.

> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index af4de817d7123..d7a077b5ccd1c 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2034,7 +2034,8 @@ static void bti_enable(const struct arm64_cpu_capabilities *__unused)
>  static void cpu_enable_mte(struct arm64_cpu_capabilities const *cap)
>  {
>  	sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ATA | SCTLR_EL1_ATA0);

I wonder if we need to keep this here on its own rather than moving it
to mte_cpu_setup(). I guess we avoid writing this register the third
time on the resume path.

> -	isb();
> +
> +	mte_cpu_setup();
>  

So in principle I'm fine with moving the initialisation to C. However,
a concern I have is that we may no longer spot potential SoC issues that
trigger SErrors on tag accesses. The architecture does allow any Normal
Cacheable mapping to fetch tags, even if marked as untagged, though most
current implementations won't do this. In addition, if you run this in a
virtualised environment, a guest without this patch (or a malicious one)
can simply enable tagging in MAIR_EL1. HCR_EL2.ATA==0 does not have any
effect, so you haven't solved the problem.

So if part of RAM in your SoC does not support MTE, I don't think you
should rely on any kernel command-line options, just make sure no
untrusted guest can access memory that may trigger SErrors even if MTE
is disabled and the control registers level. Current behaviour (without
this patch) comes in handy to detect such issues.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel