On Thu, Oct 7, 2021 at 3:20 AM Mark Rutland [off-list ref] wrote:

On Tue, Oct 05, 2021 at 12:08:58PM -0700, Peter Collingbourne wrote:

quoted

On Tue, Oct 5, 2021 at 9:46 AM Catalin Marinas [off-list ref] wrote:

quoted

On Wed, Sep 29, 2021 at 12:45:24PM -0700, Peter Collingbourne wrote:

quoted

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 2f69ae43941d..85ead6bbb38e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S

@@ -269,7 +269,28 @@ alternative_else_nop_endif
      .else
      add     x21, sp, #PT_REGS_SIZE
      get_current_task tsk
+     ldr     x0, [tsk, THREAD_SCTLR_USER]
      .endif /* \el == 0 */
+
+     /*
+      * Re-enable tag checking (TCO set on exception entry). This is only
+      * necessary if MTE is enabled in either the kernel or the userspace
+      * task in synchronous mode. With MTE disabled in the kernel and
+      * disabled or asynchronous in userspace, tag check faults (including in
+      * uaccesses) are not reported, therefore there is no need to re-enable
+      * checking. This is beneficial on microarchitectures where re-enabling
+      * TCO is expensive.
+      */
+#ifdef CONFIG_ARM64_MTE
+alternative_cb       kasan_hw_tags_enable
+     tbz     x0, #SCTLR_EL1_TCF0_SHIFT, 1f
+alternative_cb_end
+alternative_if ARM64_MTE
+     SET_PSTATE_TCO(0)
+alternative_else_nop_endif
+1:
+#endif

I think we can get here from an interrupt as well. Can we guarantee that
the sctlr_user is valid? We are not always in a user process context.

Looking through the code in entry.S carefully it doesn't appear that
the tsk pointer is ever used when taking an exception from EL1. The
last user appears to have been removed in commit 3d2403fd10a1 ("arm64:
uaccess: remove set_fs()"). I just did a quick boot test on a couple
of platforms and things seem to work without the "get_current_task
tsk" line.

So I can't be confident that tsk points to a valid task, but at least
it looks like it was the case prior to that commit.

The EL1 entries unconditionally call enter_from_kernel_mode() which
(at least in some configurations) unconditionally accesses current, so
I'm reasonably confident that we have a valid current pointer here.

quoted

quoted

Maybe only do the above checks if \el == 0, otherwise just bracket it
with kasan_hw_tags_enable.

Is it possible for us to do a uaccess inside the EL1 -> EL1 exception
handler? That was the one thing I was unsure about and it's why I
opted to do the same check regardless of EL.

Today we can do so if we take a PMU IRQ from EL1 and try to do an EL0
stack unwind or stack dump, and similar holds for some eBPF stuff.

We also need to ensure that PSTATE.TCO is configured consistently so
that context-switch works, otherwise where a CPU switches between tasks
A and B, where A is preempted by an EL1 IRQ, and B is explicitly
switching via a direct call to schedule(), the state of TCO will not be
as expected (unless we track this per thread, and handle it in the
context switch).

Isn't this already context switched via storing the per-task SPSR?
Otherwise e.g. the TCO controls in __uaccess_disable_tco() and
__uaccess_enable_tco() would have the same problem.

I'd strongly prefer that the state of PSTATE.TCO upon taking an
exception to EL1 is consistent (by the end of the early entry code),
regardless of where that was taken from.

Is it easier to manage this from within entry-common.c?

It doesn't look like we have any common location to add code that runs
on every kernel entry in entry-common.c. I tried adding one (patch
below), but this ended up leading to a performance regression (versus
baseline) on some of the cores on the hardware that I have access to.

Peter

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 64cfe4a3798f..ad066b09a6b7 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c

@@ -17,6 +17,23 @@
 #include <asm/mmu.h>
 #include <asm/sysreg.h>

+static void mte_disable_tco_entry(void)
+{
+ /*
+ * Re-enable tag checking (TCO set on exception entry). This is only
+ * necessary if MTE is enabled in either the kernel or the userspace
+ * task in synchronous mode. With MTE disabled in the kernel and
+ * disabled or asynchronous in userspace, tag check faults (including in
+ * uaccesses) are not reported, therefore there is no need to re-enable
+ * checking. This is beneficial on microarchitectures where re-enabling
+ * TCO is expensive.
+ */
+ if (IS_ENABLED(CONFIG_ARM64_MTE) &&
+     (kasan_hw_tags_enabled() ||
+      (current->thread.sctlr_user & (1UL << SCTLR_EL1_TCF0_SHIFT))))
+ asm volatile(SET_PSTATE_TCO(0));
+}
+
 /*
  * This is intended to match the logic in irqentry_enter(), handling the kernel
  * mode transitions only.

@@ -39,6 +56,7 @@ static void noinstr enter_from_kernel_mode(struct

pt_regs *regs)
  trace_hardirqs_off_finish();

  mte_check_tfsr_entry();
+ mte_disable_tco_entry();
 }

 /*

@@ -235,6 +253,7 @@ asmlinkage void noinstr enter_from_user_mode(void)
  CT_WARN_ON(ct_state() != CONTEXT_USER);
  user_exit_irqoff();
  trace_hardirqs_off_finish();
+ mte_disable_tco_entry();
 }

 asmlinkage void noinstr exit_to_user_mode(void)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 42c404d508ca..8c63279ffea7 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S

@@ -339,13 +339,6 @@ alternative_if ARM64_HAS_IRQ_PRIO_MASKING
  msr_s SYS_ICC_PMR_EL1, x20
 alternative_else_nop_endif

- /* Re-enable tag checking (TCO set on exception entry) */
-#ifdef CONFIG_ARM64_MTE
-alternative_if ARM64_MTE
- SET_PSTATE_TCO(0)
-alternative_else_nop_endif
-#endif
-
  /*
  * Registers that may be useful after this macro is invoked:
  *

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