All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
@ 2021-04-09 13:24 ` Vincenzo Frascino
  0 siblings, 0 replies; 10+ messages in thread
From: Vincenzo Frascino @ 2021-04-09 13:24 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, kasan-dev
  Cc: Vincenzo Frascino, Catalin Marinas, Will Deacon, stable

The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
race with another CPU doing a set_tsk_thread_flag() and all the other flags
can be lost in the process.

Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
exit_to_user_mode() to address the problem.

Note: Moving the check in entry-common allows to use set_thread_flag()
which is safe.

Fixes: 637ec831ea4f ("arm64: mte: Handle synchronous and asynchronous tag check faults")
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: stable@vger.kernel.org
Reported-by: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h     |  9 +++++++++
 arch/arm64/kernel/entry-common.c |  6 ++++++
 arch/arm64/kernel/entry.S        | 34 --------------------------------
 arch/arm64/kernel/mte.c          | 33 +++++++++++++++++++++++++++++--
 4 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 9b557a457f24..c7ab681a95c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -49,6 +49,9 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
 
 void mte_assign_mem_tag_range(void *addr, size_t size);
 
+void noinstr check_mte_async_tcf0(void);
+void noinstr clear_mte_async_tcf0(void);
+
 #else /* CONFIG_ARM64_MTE */
 
 /* unused if !CONFIG_ARM64_MTE, silence the compiler */
@@ -83,6 +86,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
 {
 	return -EIO;
 }
+static inline void check_mte_async_tcf0(void)
+{
+}
+static inline void clear_mte_async_tcf0(void)
+{
+}
 
 static inline void mte_assign_mem_tag_range(void *addr, size_t size)
 {
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 9d3588450473..837d3624a1d5 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -289,10 +289,16 @@ asmlinkage void noinstr enter_from_user_mode(void)
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
 	user_exit_irqoff();
 	trace_hardirqs_off_finish();
+
+	/* Check for asynchronous tag check faults in user space */
+	check_mte_async_tcf0();
 }
 
 asmlinkage void noinstr exit_to_user_mode(void)
 {
+	/* Ignore asynchronous tag check faults in the uaccess routines */
+	clear_mte_async_tcf0();
+
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	user_enter_irqoff();
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index a31a0a713c85..fb57df0d453f 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -34,15 +34,11 @@
  * user and kernel mode.
  */
 	.macro user_exit_irqoff
-#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
 	bl	enter_from_user_mode
-#endif
 	.endm
 
 	.macro user_enter_irqoff
-#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
 	bl	exit_to_user_mode
-#endif
 	.endm
 
 	.macro	clear_gp_regs
@@ -147,32 +143,6 @@ alternative_cb_end
 .L__asm_ssbd_skip\@:
 	.endm
 
-	/* Check for MTE asynchronous tag check faults */
-	.macro check_mte_async_tcf, flgs, tmp
-#ifdef CONFIG_ARM64_MTE
-alternative_if_not ARM64_MTE
-	b	1f
-alternative_else_nop_endif
-	mrs_s	\tmp, SYS_TFSRE0_EL1
-	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
-	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
-	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
-	str	\flgs, [tsk, #TSK_TI_FLAGS]
-	msr_s	SYS_TFSRE0_EL1, xzr
-1:
-#endif
-	.endm
-
-	/* Clear the MTE asynchronous tag check faults */
-	.macro clear_mte_async_tcf
-#ifdef CONFIG_ARM64_MTE
-alternative_if ARM64_MTE
-	dsb	ish
-	msr_s	SYS_TFSRE0_EL1, xzr
-alternative_else_nop_endif
-#endif
-	.endm
-
 	.macro mte_set_gcr, tmp, tmp2
 #ifdef CONFIG_ARM64_MTE
 	/*
@@ -243,8 +213,6 @@ alternative_else_nop_endif
 	ldr	x19, [tsk, #TSK_TI_FLAGS]
 	disable_step_tsk x19, x20
 
-	/* Check for asynchronous tag check faults in user space */
-	check_mte_async_tcf x19, x22
 	apply_ssbd 1, x22, x23
 
 	ptrauth_keys_install_kernel tsk, x20, x22, x23
@@ -775,8 +743,6 @@ SYM_CODE_START_LOCAL(ret_to_user)
 	cbnz	x2, work_pending
 finish_ret_to_user:
 	user_enter_irqoff
-	/* Ignore asynchronous tag check faults in the uaccess routines */
-	clear_mte_async_tcf
 	enable_step_tsk x19, x2
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 	bl	stackleak_erase
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index b3c70a612c7a..84a942c25870 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
 	 */
 }
 
-void flush_mte_state(void)
+void noinstr check_mte_async_tcf0(void)
+{
+	u64 tcf0;
+
+	if (!system_supports_mte())
+		return;
+
+	/*
+	 * dsb(ish) is not required before the register read
+	 * because the TFSRE0_EL1 is automatically synchronized
+	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
+	 * is set.
+	 */
+	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
+
+	if (tcf0 & SYS_TFSR_EL1_TF0)
+		set_thread_flag(TIF_MTE_ASYNC_FAULT);
+
+	write_sysreg_s(0, SYS_TFSRE0_EL1);
+}
+
+void noinstr clear_mte_async_tcf0(void)
 {
 	if (!system_supports_mte())
 		return;
 
-	/* clear any pending asynchronous tag fault */
 	dsb(ish);
 	write_sysreg_s(0, SYS_TFSRE0_EL1);
+}
+
+void flush_mte_state(void)
+{
+	if (!system_supports_mte())
+		return;
+
+	/* clear any pending asynchronous tag fault */
+	clear_mte_async_tcf0();
 	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
 	/* disable tag checking */
 	set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
@ 2021-04-09 13:24 ` Vincenzo Frascino
  0 siblings, 0 replies; 10+ messages in thread
From: Vincenzo Frascino @ 2021-04-09 13:24 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, kasan-dev
  Cc: Vincenzo Frascino, Catalin Marinas, Will Deacon, stable

The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
race with another CPU doing a set_tsk_thread_flag() and all the other flags
can be lost in the process.

Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
exit_to_user_mode() to address the problem.

Note: Moving the check in entry-common allows to use set_thread_flag()
which is safe.

Fixes: 637ec831ea4f ("arm64: mte: Handle synchronous and asynchronous tag check faults")
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: stable@vger.kernel.org
Reported-by: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h     |  9 +++++++++
 arch/arm64/kernel/entry-common.c |  6 ++++++
 arch/arm64/kernel/entry.S        | 34 --------------------------------
 arch/arm64/kernel/mte.c          | 33 +++++++++++++++++++++++++++++--
 4 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 9b557a457f24..c7ab681a95c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -49,6 +49,9 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
 
 void mte_assign_mem_tag_range(void *addr, size_t size);
 
+void noinstr check_mte_async_tcf0(void);
+void noinstr clear_mte_async_tcf0(void);
+
 #else /* CONFIG_ARM64_MTE */
 
 /* unused if !CONFIG_ARM64_MTE, silence the compiler */
@@ -83,6 +86,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
 {
 	return -EIO;
 }
+static inline void check_mte_async_tcf0(void)
+{
+}
+static inline void clear_mte_async_tcf0(void)
+{
+}
 
 static inline void mte_assign_mem_tag_range(void *addr, size_t size)
 {
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 9d3588450473..837d3624a1d5 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -289,10 +289,16 @@ asmlinkage void noinstr enter_from_user_mode(void)
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
 	user_exit_irqoff();
 	trace_hardirqs_off_finish();
+
+	/* Check for asynchronous tag check faults in user space */
+	check_mte_async_tcf0();
 }
 
 asmlinkage void noinstr exit_to_user_mode(void)
 {
+	/* Ignore asynchronous tag check faults in the uaccess routines */
+	clear_mte_async_tcf0();
+
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	user_enter_irqoff();
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index a31a0a713c85..fb57df0d453f 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -34,15 +34,11 @@
  * user and kernel mode.
  */
 	.macro user_exit_irqoff
-#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
 	bl	enter_from_user_mode
-#endif
 	.endm
 
 	.macro user_enter_irqoff
-#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
 	bl	exit_to_user_mode
-#endif
 	.endm
 
 	.macro	clear_gp_regs
@@ -147,32 +143,6 @@ alternative_cb_end
 .L__asm_ssbd_skip\@:
 	.endm
 
-	/* Check for MTE asynchronous tag check faults */
-	.macro check_mte_async_tcf, flgs, tmp
-#ifdef CONFIG_ARM64_MTE
-alternative_if_not ARM64_MTE
-	b	1f
-alternative_else_nop_endif
-	mrs_s	\tmp, SYS_TFSRE0_EL1
-	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
-	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
-	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
-	str	\flgs, [tsk, #TSK_TI_FLAGS]
-	msr_s	SYS_TFSRE0_EL1, xzr
-1:
-#endif
-	.endm
-
-	/* Clear the MTE asynchronous tag check faults */
-	.macro clear_mte_async_tcf
-#ifdef CONFIG_ARM64_MTE
-alternative_if ARM64_MTE
-	dsb	ish
-	msr_s	SYS_TFSRE0_EL1, xzr
-alternative_else_nop_endif
-#endif
-	.endm
-
 	.macro mte_set_gcr, tmp, tmp2
 #ifdef CONFIG_ARM64_MTE
 	/*
@@ -243,8 +213,6 @@ alternative_else_nop_endif
 	ldr	x19, [tsk, #TSK_TI_FLAGS]
 	disable_step_tsk x19, x20
 
-	/* Check for asynchronous tag check faults in user space */
-	check_mte_async_tcf x19, x22
 	apply_ssbd 1, x22, x23
 
 	ptrauth_keys_install_kernel tsk, x20, x22, x23
@@ -775,8 +743,6 @@ SYM_CODE_START_LOCAL(ret_to_user)
 	cbnz	x2, work_pending
 finish_ret_to_user:
 	user_enter_irqoff
-	/* Ignore asynchronous tag check faults in the uaccess routines */
-	clear_mte_async_tcf
 	enable_step_tsk x19, x2
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 	bl	stackleak_erase
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index b3c70a612c7a..84a942c25870 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
 	 */
 }
 
-void flush_mte_state(void)
+void noinstr check_mte_async_tcf0(void)
+{
+	u64 tcf0;
+
+	if (!system_supports_mte())
+		return;
+
+	/*
+	 * dsb(ish) is not required before the register read
+	 * because the TFSRE0_EL1 is automatically synchronized
+	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
+	 * is set.
+	 */
+	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
+
+	if (tcf0 & SYS_TFSR_EL1_TF0)
+		set_thread_flag(TIF_MTE_ASYNC_FAULT);
+
+	write_sysreg_s(0, SYS_TFSRE0_EL1);
+}
+
+void noinstr clear_mte_async_tcf0(void)
 {
 	if (!system_supports_mte())
 		return;
 
-	/* clear any pending asynchronous tag fault */
 	dsb(ish);
 	write_sysreg_s(0, SYS_TFSRE0_EL1);
+}
+
+void flush_mte_state(void)
+{
+	if (!system_supports_mte())
+		return;
+
+	/* clear any pending asynchronous tag fault */
+	clear_mte_async_tcf0();
 	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
 	/* disable tag checking */
 	set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
  2021-04-09 13:24 ` Vincenzo Frascino
@ 2021-04-09 14:32   ` Mark Rutland
  -1 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2021-04-09 14:32 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: linux-arm-kernel, linux-kernel, kasan-dev, Catalin Marinas,
	Will Deacon, stable

Hi Vincenzo,

On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
> race with another CPU doing a set_tsk_thread_flag() and all the other flags
> can be lost in the process.
> 
> Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
> exit_to_user_mode() to address the problem.
> 
> Note: Moving the check in entry-common allows to use set_thread_flag()
> which is safe.
> 
> Fixes: 637ec831ea4f ("arm64: mte: Handle synchronous and asynchronous tag check faults")
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/mte.h     |  9 +++++++++
>  arch/arm64/kernel/entry-common.c |  6 ++++++
>  arch/arm64/kernel/entry.S        | 34 --------------------------------
>  arch/arm64/kernel/mte.c          | 33 +++++++++++++++++++++++++++++--
>  4 files changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 9b557a457f24..c7ab681a95c3 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -49,6 +49,9 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
>  
>  void mte_assign_mem_tag_range(void *addr, size_t size);
>  
> +void noinstr check_mte_async_tcf0(void);
> +void noinstr clear_mte_async_tcf0(void);

Can we please put the implementations in the header so that they can be
inlined? Otherwise when the HW doesn't support MTE we'll always do a pointless
branch to the out-of-line implementation.

With that, we can mark them __always_inline to avoid weirdness with an inline
noinstr function.

Otherwise, this looks good to me.

Thanks,
Mark.

> +
>  #else /* CONFIG_ARM64_MTE */
>  
>  /* unused if !CONFIG_ARM64_MTE, silence the compiler */
> @@ -83,6 +86,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
>  {
>  	return -EIO;
>  }
> +static inline void check_mte_async_tcf0(void)
> +{
> +}
> +static inline void clear_mte_async_tcf0(void)
> +{
> +}
>  
>  static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>  {
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 9d3588450473..837d3624a1d5 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -289,10 +289,16 @@ asmlinkage void noinstr enter_from_user_mode(void)
>  	CT_WARN_ON(ct_state() != CONTEXT_USER);
>  	user_exit_irqoff();
>  	trace_hardirqs_off_finish();
> +
> +	/* Check for asynchronous tag check faults in user space */
> +	check_mte_async_tcf0();



>  }
>  
>  asmlinkage void noinstr exit_to_user_mode(void)
>  {
> +	/* Ignore asynchronous tag check faults in the uaccess routines */
> +	clear_mte_async_tcf0();
> +
>  	trace_hardirqs_on_prepare();
>  	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
>  	user_enter_irqoff();
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index a31a0a713c85..fb57df0d453f 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -34,15 +34,11 @@
>   * user and kernel mode.
>   */
>  	.macro user_exit_irqoff
> -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
>  	bl	enter_from_user_mode
> -#endif
>  	.endm
>  
>  	.macro user_enter_irqoff
> -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
>  	bl	exit_to_user_mode
> -#endif
>  	.endm
>  
>  	.macro	clear_gp_regs
> @@ -147,32 +143,6 @@ alternative_cb_end
>  .L__asm_ssbd_skip\@:
>  	.endm
>  
> -	/* Check for MTE asynchronous tag check faults */
> -	.macro check_mte_async_tcf, flgs, tmp
> -#ifdef CONFIG_ARM64_MTE
> -alternative_if_not ARM64_MTE
> -	b	1f
> -alternative_else_nop_endif
> -	mrs_s	\tmp, SYS_TFSRE0_EL1
> -	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
> -	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
> -	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
> -	str	\flgs, [tsk, #TSK_TI_FLAGS]
> -	msr_s	SYS_TFSRE0_EL1, xzr
> -1:
> -#endif
> -	.endm
> -
> -	/* Clear the MTE asynchronous tag check faults */
> -	.macro clear_mte_async_tcf
> -#ifdef CONFIG_ARM64_MTE
> -alternative_if ARM64_MTE
> -	dsb	ish
> -	msr_s	SYS_TFSRE0_EL1, xzr
> -alternative_else_nop_endif
> -#endif
> -	.endm
> -
>  	.macro mte_set_gcr, tmp, tmp2
>  #ifdef CONFIG_ARM64_MTE
>  	/*
> @@ -243,8 +213,6 @@ alternative_else_nop_endif
>  	ldr	x19, [tsk, #TSK_TI_FLAGS]
>  	disable_step_tsk x19, x20
>  
> -	/* Check for asynchronous tag check faults in user space */
> -	check_mte_async_tcf x19, x22
>  	apply_ssbd 1, x22, x23
>  
>  	ptrauth_keys_install_kernel tsk, x20, x22, x23
> @@ -775,8 +743,6 @@ SYM_CODE_START_LOCAL(ret_to_user)
>  	cbnz	x2, work_pending
>  finish_ret_to_user:
>  	user_enter_irqoff
> -	/* Ignore asynchronous tag check faults in the uaccess routines */
> -	clear_mte_async_tcf
>  	enable_step_tsk x19, x2
>  #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>  	bl	stackleak_erase
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index b3c70a612c7a..84a942c25870 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
>  	 */
>  }
>  
> -void flush_mte_state(void)
> +void noinstr check_mte_async_tcf0(void)
> +{
> +	u64 tcf0;
> +
> +	if (!system_supports_mte())
> +		return;
> +
> +	/*
> +	 * dsb(ish) is not required before the register read
> +	 * because the TFSRE0_EL1 is automatically synchronized
> +	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
> +	 * is set.
> +	 */
> +	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
> +
> +	if (tcf0 & SYS_TFSR_EL1_TF0)
> +		set_thread_flag(TIF_MTE_ASYNC_FAULT);
> +
> +	write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}
> +
> +void noinstr clear_mte_async_tcf0(void)
>  {
>  	if (!system_supports_mte())
>  		return;
>  
> -	/* clear any pending asynchronous tag fault */
>  	dsb(ish);
>  	write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}
> +
> +void flush_mte_state(void)
> +{
> +	if (!system_supports_mte())
> +		return;
> +
> +	/* clear any pending asynchronous tag fault */
> +	clear_mte_async_tcf0();
>  	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
>  	/* disable tag checking */
>  	set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
@ 2021-04-09 14:32   ` Mark Rutland
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2021-04-09 14:32 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: linux-arm-kernel, linux-kernel, kasan-dev, Catalin Marinas,
	Will Deacon, stable

Hi Vincenzo,

On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
> race with another CPU doing a set_tsk_thread_flag() and all the other flags
> can be lost in the process.
> 
> Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
> exit_to_user_mode() to address the problem.
> 
> Note: Moving the check in entry-common allows to use set_thread_flag()
> which is safe.
> 
> Fixes: 637ec831ea4f ("arm64: mte: Handle synchronous and asynchronous tag check faults")
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/mte.h     |  9 +++++++++
>  arch/arm64/kernel/entry-common.c |  6 ++++++
>  arch/arm64/kernel/entry.S        | 34 --------------------------------
>  arch/arm64/kernel/mte.c          | 33 +++++++++++++++++++++++++++++--
>  4 files changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 9b557a457f24..c7ab681a95c3 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -49,6 +49,9 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
>  
>  void mte_assign_mem_tag_range(void *addr, size_t size);
>  
> +void noinstr check_mte_async_tcf0(void);
> +void noinstr clear_mte_async_tcf0(void);

Can we please put the implementations in the header so that they can be
inlined? Otherwise when the HW doesn't support MTE we'll always do a pointless
branch to the out-of-line implementation.

With that, we can mark them __always_inline to avoid weirdness with an inline
noinstr function.

Otherwise, this looks good to me.

Thanks,
Mark.

> +
>  #else /* CONFIG_ARM64_MTE */
>  
>  /* unused if !CONFIG_ARM64_MTE, silence the compiler */
> @@ -83,6 +86,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
>  {
>  	return -EIO;
>  }
> +static inline void check_mte_async_tcf0(void)
> +{
> +}
> +static inline void clear_mte_async_tcf0(void)
> +{
> +}
>  
>  static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>  {
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 9d3588450473..837d3624a1d5 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -289,10 +289,16 @@ asmlinkage void noinstr enter_from_user_mode(void)
>  	CT_WARN_ON(ct_state() != CONTEXT_USER);
>  	user_exit_irqoff();
>  	trace_hardirqs_off_finish();
> +
> +	/* Check for asynchronous tag check faults in user space */
> +	check_mte_async_tcf0();



>  }
>  
>  asmlinkage void noinstr exit_to_user_mode(void)
>  {
> +	/* Ignore asynchronous tag check faults in the uaccess routines */
> +	clear_mte_async_tcf0();
> +
>  	trace_hardirqs_on_prepare();
>  	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
>  	user_enter_irqoff();
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index a31a0a713c85..fb57df0d453f 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -34,15 +34,11 @@
>   * user and kernel mode.
>   */
>  	.macro user_exit_irqoff
> -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
>  	bl	enter_from_user_mode
> -#endif
>  	.endm
>  
>  	.macro user_enter_irqoff
> -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
>  	bl	exit_to_user_mode
> -#endif
>  	.endm
>  
>  	.macro	clear_gp_regs
> @@ -147,32 +143,6 @@ alternative_cb_end
>  .L__asm_ssbd_skip\@:
>  	.endm
>  
> -	/* Check for MTE asynchronous tag check faults */
> -	.macro check_mte_async_tcf, flgs, tmp
> -#ifdef CONFIG_ARM64_MTE
> -alternative_if_not ARM64_MTE
> -	b	1f
> -alternative_else_nop_endif
> -	mrs_s	\tmp, SYS_TFSRE0_EL1
> -	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
> -	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
> -	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
> -	str	\flgs, [tsk, #TSK_TI_FLAGS]
> -	msr_s	SYS_TFSRE0_EL1, xzr
> -1:
> -#endif
> -	.endm
> -
> -	/* Clear the MTE asynchronous tag check faults */
> -	.macro clear_mte_async_tcf
> -#ifdef CONFIG_ARM64_MTE
> -alternative_if ARM64_MTE
> -	dsb	ish
> -	msr_s	SYS_TFSRE0_EL1, xzr
> -alternative_else_nop_endif
> -#endif
> -	.endm
> -
>  	.macro mte_set_gcr, tmp, tmp2
>  #ifdef CONFIG_ARM64_MTE
>  	/*
> @@ -243,8 +213,6 @@ alternative_else_nop_endif
>  	ldr	x19, [tsk, #TSK_TI_FLAGS]
>  	disable_step_tsk x19, x20
>  
> -	/* Check for asynchronous tag check faults in user space */
> -	check_mte_async_tcf x19, x22
>  	apply_ssbd 1, x22, x23
>  
>  	ptrauth_keys_install_kernel tsk, x20, x22, x23
> @@ -775,8 +743,6 @@ SYM_CODE_START_LOCAL(ret_to_user)
>  	cbnz	x2, work_pending
>  finish_ret_to_user:
>  	user_enter_irqoff
> -	/* Ignore asynchronous tag check faults in the uaccess routines */
> -	clear_mte_async_tcf
>  	enable_step_tsk x19, x2
>  #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>  	bl	stackleak_erase
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index b3c70a612c7a..84a942c25870 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
>  	 */
>  }
>  
> -void flush_mte_state(void)
> +void noinstr check_mte_async_tcf0(void)
> +{
> +	u64 tcf0;
> +
> +	if (!system_supports_mte())
> +		return;
> +
> +	/*
> +	 * dsb(ish) is not required before the register read
> +	 * because the TFSRE0_EL1 is automatically synchronized
> +	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
> +	 * is set.
> +	 */
> +	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
> +
> +	if (tcf0 & SYS_TFSR_EL1_TF0)
> +		set_thread_flag(TIF_MTE_ASYNC_FAULT);
> +
> +	write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}
> +
> +void noinstr clear_mte_async_tcf0(void)
>  {
>  	if (!system_supports_mte())
>  		return;
>  
> -	/* clear any pending asynchronous tag fault */
>  	dsb(ish);
>  	write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}
> +
> +void flush_mte_state(void)
> +{
> +	if (!system_supports_mte())
> +		return;
> +
> +	/* clear any pending asynchronous tag fault */
> +	clear_mte_async_tcf0();
>  	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
>  	/* disable tag checking */
>  	set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
> -- 
> 2.30.2
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
  2021-04-09 13:24 ` Vincenzo Frascino
@ 2021-04-09 14:47   ` Catalin Marinas
  -1 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2021-04-09 14:47 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: linux-arm-kernel, linux-kernel, kasan-dev, Will Deacon, stable

On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index b3c70a612c7a..84a942c25870 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
>  	 */
>  }
>  
> -void flush_mte_state(void)
> +void noinstr check_mte_async_tcf0(void)

Nitpick: it looks like naming isn't be entirely consistent with your
kernel async patches:

https://lore.kernel.org/linux-arm-kernel/20210315132019.33202-8-vincenzo.frascino@arm.com/

You could name them mte_check_tfsre0_el1() etc. Also make sure they are
called in similar places in both series.

> +{
> +	u64 tcf0;
> +
> +	if (!system_supports_mte())
> +		return;
> +
> +	/*
> +	 * dsb(ish) is not required before the register read
> +	 * because the TFSRE0_EL1 is automatically synchronized
> +	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
> +	 * is set.
> +	 */
> +	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
> +
> +	if (tcf0 & SYS_TFSR_EL1_TF0)
> +		set_thread_flag(TIF_MTE_ASYNC_FAULT);
> +
> +	write_sysreg_s(0, SYS_TFSRE0_EL1);

Please move the write_sysreg() inside the 'if' block. If it was 0,
there's no point in a potentially more expensive write.

That said, we only check TFSRE0_EL1 on entry from EL0. Is there a point
in clearing it before we return to EL0? Uaccess routines may set it
anyway.

> +}
> +
> +void noinstr clear_mte_async_tcf0(void)
>  {
>  	if (!system_supports_mte())
>  		return;
>  
> -	/* clear any pending asynchronous tag fault */
>  	dsb(ish);
>  	write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}

I think Mark suggested on your first version that we should keep these
functions in mte.h so that they can be inlined. They are small and only
called in one or two places.

-- 
Catalin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
@ 2021-04-09 14:47   ` Catalin Marinas
  0 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2021-04-09 14:47 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: linux-arm-kernel, linux-kernel, kasan-dev, Will Deacon, stable

On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index b3c70a612c7a..84a942c25870 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
>  	 */
>  }
>  
> -void flush_mte_state(void)
> +void noinstr check_mte_async_tcf0(void)

Nitpick: it looks like naming isn't be entirely consistent with your
kernel async patches:

https://lore.kernel.org/linux-arm-kernel/20210315132019.33202-8-vincenzo.frascino@arm.com/

You could name them mte_check_tfsre0_el1() etc. Also make sure they are
called in similar places in both series.

> +{
> +	u64 tcf0;
> +
> +	if (!system_supports_mte())
> +		return;
> +
> +	/*
> +	 * dsb(ish) is not required before the register read
> +	 * because the TFSRE0_EL1 is automatically synchronized
> +	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
> +	 * is set.
> +	 */
> +	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
> +
> +	if (tcf0 & SYS_TFSR_EL1_TF0)
> +		set_thread_flag(TIF_MTE_ASYNC_FAULT);
> +
> +	write_sysreg_s(0, SYS_TFSRE0_EL1);

Please move the write_sysreg() inside the 'if' block. If it was 0,
there's no point in a potentially more expensive write.

That said, we only check TFSRE0_EL1 on entry from EL0. Is there a point
in clearing it before we return to EL0? Uaccess routines may set it
anyway.

> +}
> +
> +void noinstr clear_mte_async_tcf0(void)
>  {
>  	if (!system_supports_mte())
>  		return;
>  
> -	/* clear any pending asynchronous tag fault */
>  	dsb(ish);
>  	write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}

I think Mark suggested on your first version that we should keep these
functions in mte.h so that they can be inlined. They are small and only
called in one or two places.

-- 
Catalin

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
  2021-04-09 14:32   ` Mark Rutland
@ 2021-04-09 16:18     ` Mark Rutland
  -1 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2021-04-09 16:18 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: linux-arm-kernel, linux-kernel, kasan-dev, Catalin Marinas,
	Will Deacon, stable

On Fri, Apr 09, 2021 at 03:32:47PM +0100, Mark Rutland wrote:
> Hi Vincenzo,
> 
> On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> > The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
> > race with another CPU doing a set_tsk_thread_flag() and all the other flags
> > can be lost in the process.
> > 
> > Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
> > exit_to_user_mode() to address the problem.
> > 
> > Note: Moving the check in entry-common allows to use set_thread_flag()
> > which is safe.

I've dug into this a bit more, and as set_thread_flag() calls some
potentially-instrumented helpers I don't think this is safe after all
(as e.g. those might cause an EL1 exception and clobber the ESR/FAR/etc
before the EL0 exception handler reads it).

Making that watertight is pretty hairy, as we either need to open-code
set_thread_flag() or go rework a load of core code. If we can use STSET
in the entry asm that'd be simpler, otherwise we'll need something more
involved.

Thanks,
Mark.

> > 
> > Fixes: 637ec831ea4f ("arm64: mte: Handle synchronous and asynchronous tag check faults")
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: stable@vger.kernel.org
> > Reported-by: Will Deacon <will@kernel.org>
> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > ---
> >  arch/arm64/include/asm/mte.h     |  9 +++++++++
> >  arch/arm64/kernel/entry-common.c |  6 ++++++
> >  arch/arm64/kernel/entry.S        | 34 --------------------------------
> >  arch/arm64/kernel/mte.c          | 33 +++++++++++++++++++++++++++++--
> >  4 files changed, 46 insertions(+), 36 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> > index 9b557a457f24..c7ab681a95c3 100644
> > --- a/arch/arm64/include/asm/mte.h
> > +++ b/arch/arm64/include/asm/mte.h
> > @@ -49,6 +49,9 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
> >  
> >  void mte_assign_mem_tag_range(void *addr, size_t size);
> >  
> > +void noinstr check_mte_async_tcf0(void);
> > +void noinstr clear_mte_async_tcf0(void);
> 
> Can we please put the implementations in the header so that they can be
> inlined? Otherwise when the HW doesn't support MTE we'll always do a pointless
> branch to the out-of-line implementation.
> 
> With that, we can mark them __always_inline to avoid weirdness with an inline
> noinstr function.
> 
> Otherwise, this looks good to me.
> 
> Thanks,
> Mark.
> 
> > +
> >  #else /* CONFIG_ARM64_MTE */
> >  
> >  /* unused if !CONFIG_ARM64_MTE, silence the compiler */
> > @@ -83,6 +86,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
> >  {
> >  	return -EIO;
> >  }
> > +static inline void check_mte_async_tcf0(void)
> > +{
> > +}
> > +static inline void clear_mte_async_tcf0(void)
> > +{
> > +}
> >  
> >  static inline void mte_assign_mem_tag_range(void *addr, size_t size)
> >  {
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index 9d3588450473..837d3624a1d5 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -289,10 +289,16 @@ asmlinkage void noinstr enter_from_user_mode(void)
> >  	CT_WARN_ON(ct_state() != CONTEXT_USER);
> >  	user_exit_irqoff();
> >  	trace_hardirqs_off_finish();
> > +
> > +	/* Check for asynchronous tag check faults in user space */
> > +	check_mte_async_tcf0();
> 
> 
> 
> >  }
> >  
> >  asmlinkage void noinstr exit_to_user_mode(void)
> >  {
> > +	/* Ignore asynchronous tag check faults in the uaccess routines */
> > +	clear_mte_async_tcf0();
> > +
> >  	trace_hardirqs_on_prepare();
> >  	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
> >  	user_enter_irqoff();
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index a31a0a713c85..fb57df0d453f 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -34,15 +34,11 @@
> >   * user and kernel mode.
> >   */
> >  	.macro user_exit_irqoff
> > -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
> >  	bl	enter_from_user_mode
> > -#endif
> >  	.endm
> >  
> >  	.macro user_enter_irqoff
> > -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
> >  	bl	exit_to_user_mode
> > -#endif
> >  	.endm
> >  
> >  	.macro	clear_gp_regs
> > @@ -147,32 +143,6 @@ alternative_cb_end
> >  .L__asm_ssbd_skip\@:
> >  	.endm
> >  
> > -	/* Check for MTE asynchronous tag check faults */
> > -	.macro check_mte_async_tcf, flgs, tmp
> > -#ifdef CONFIG_ARM64_MTE
> > -alternative_if_not ARM64_MTE
> > -	b	1f
> > -alternative_else_nop_endif
> > -	mrs_s	\tmp, SYS_TFSRE0_EL1
> > -	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
> > -	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
> > -	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
> > -	str	\flgs, [tsk, #TSK_TI_FLAGS]
> > -	msr_s	SYS_TFSRE0_EL1, xzr
> > -1:
> > -#endif
> > -	.endm
> > -
> > -	/* Clear the MTE asynchronous tag check faults */
> > -	.macro clear_mte_async_tcf
> > -#ifdef CONFIG_ARM64_MTE
> > -alternative_if ARM64_MTE
> > -	dsb	ish
> > -	msr_s	SYS_TFSRE0_EL1, xzr
> > -alternative_else_nop_endif
> > -#endif
> > -	.endm
> > -
> >  	.macro mte_set_gcr, tmp, tmp2
> >  #ifdef CONFIG_ARM64_MTE
> >  	/*
> > @@ -243,8 +213,6 @@ alternative_else_nop_endif
> >  	ldr	x19, [tsk, #TSK_TI_FLAGS]
> >  	disable_step_tsk x19, x20
> >  
> > -	/* Check for asynchronous tag check faults in user space */
> > -	check_mte_async_tcf x19, x22
> >  	apply_ssbd 1, x22, x23
> >  
> >  	ptrauth_keys_install_kernel tsk, x20, x22, x23
> > @@ -775,8 +743,6 @@ SYM_CODE_START_LOCAL(ret_to_user)
> >  	cbnz	x2, work_pending
> >  finish_ret_to_user:
> >  	user_enter_irqoff
> > -	/* Ignore asynchronous tag check faults in the uaccess routines */
> > -	clear_mte_async_tcf
> >  	enable_step_tsk x19, x2
> >  #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> >  	bl	stackleak_erase
> > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index b3c70a612c7a..84a942c25870 100644
> > --- a/arch/arm64/kernel/mte.c
> > +++ b/arch/arm64/kernel/mte.c
> > @@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
> >  	 */
> >  }
> >  
> > -void flush_mte_state(void)
> > +void noinstr check_mte_async_tcf0(void)
> > +{
> > +	u64 tcf0;
> > +
> > +	if (!system_supports_mte())
> > +		return;
> > +
> > +	/*
> > +	 * dsb(ish) is not required before the register read
> > +	 * because the TFSRE0_EL1 is automatically synchronized
> > +	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
> > +	 * is set.
> > +	 */
> > +	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
> > +
> > +	if (tcf0 & SYS_TFSR_EL1_TF0)
> > +		set_thread_flag(TIF_MTE_ASYNC_FAULT);
> > +
> > +	write_sysreg_s(0, SYS_TFSRE0_EL1);
> > +}
> > +
> > +void noinstr clear_mte_async_tcf0(void)
> >  {
> >  	if (!system_supports_mte())
> >  		return;
> >  
> > -	/* clear any pending asynchronous tag fault */
> >  	dsb(ish);
> >  	write_sysreg_s(0, SYS_TFSRE0_EL1);
> > +}
> > +
> > +void flush_mte_state(void)
> > +{
> > +	if (!system_supports_mte())
> > +		return;
> > +
> > +	/* clear any pending asynchronous tag fault */
> > +	clear_mte_async_tcf0();
> >  	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
> >  	/* disable tag checking */
> >  	set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
> > -- 
> > 2.30.2
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
@ 2021-04-09 16:18     ` Mark Rutland
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2021-04-09 16:18 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: linux-arm-kernel, linux-kernel, kasan-dev, Catalin Marinas,
	Will Deacon, stable

On Fri, Apr 09, 2021 at 03:32:47PM +0100, Mark Rutland wrote:
> Hi Vincenzo,
> 
> On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> > The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
> > race with another CPU doing a set_tsk_thread_flag() and all the other flags
> > can be lost in the process.
> > 
> > Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
> > exit_to_user_mode() to address the problem.
> > 
> > Note: Moving the check in entry-common allows to use set_thread_flag()
> > which is safe.

I've dug into this a bit more, and as set_thread_flag() calls some
potentially-instrumented helpers I don't think this is safe after all
(as e.g. those might cause an EL1 exception and clobber the ESR/FAR/etc
before the EL0 exception handler reads it).

Making that watertight is pretty hairy, as we either need to open-code
set_thread_flag() or go rework a load of core code. If we can use STSET
in the entry asm that'd be simpler, otherwise we'll need something more
involved.

Thanks,
Mark.

> > 
> > Fixes: 637ec831ea4f ("arm64: mte: Handle synchronous and asynchronous tag check faults")
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: stable@vger.kernel.org
> > Reported-by: Will Deacon <will@kernel.org>
> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > ---
> >  arch/arm64/include/asm/mte.h     |  9 +++++++++
> >  arch/arm64/kernel/entry-common.c |  6 ++++++
> >  arch/arm64/kernel/entry.S        | 34 --------------------------------
> >  arch/arm64/kernel/mte.c          | 33 +++++++++++++++++++++++++++++--
> >  4 files changed, 46 insertions(+), 36 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> > index 9b557a457f24..c7ab681a95c3 100644
> > --- a/arch/arm64/include/asm/mte.h
> > +++ b/arch/arm64/include/asm/mte.h
> > @@ -49,6 +49,9 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
> >  
> >  void mte_assign_mem_tag_range(void *addr, size_t size);
> >  
> > +void noinstr check_mte_async_tcf0(void);
> > +void noinstr clear_mte_async_tcf0(void);
> 
> Can we please put the implementations in the header so that they can be
> inlined? Otherwise when the HW doesn't support MTE we'll always do a pointless
> branch to the out-of-line implementation.
> 
> With that, we can mark them __always_inline to avoid weirdness with an inline
> noinstr function.
> 
> Otherwise, this looks good to me.
> 
> Thanks,
> Mark.
> 
> > +
> >  #else /* CONFIG_ARM64_MTE */
> >  
> >  /* unused if !CONFIG_ARM64_MTE, silence the compiler */
> > @@ -83,6 +86,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
> >  {
> >  	return -EIO;
> >  }
> > +static inline void check_mte_async_tcf0(void)
> > +{
> > +}
> > +static inline void clear_mte_async_tcf0(void)
> > +{
> > +}
> >  
> >  static inline void mte_assign_mem_tag_range(void *addr, size_t size)
> >  {
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index 9d3588450473..837d3624a1d5 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -289,10 +289,16 @@ asmlinkage void noinstr enter_from_user_mode(void)
> >  	CT_WARN_ON(ct_state() != CONTEXT_USER);
> >  	user_exit_irqoff();
> >  	trace_hardirqs_off_finish();
> > +
> > +	/* Check for asynchronous tag check faults in user space */
> > +	check_mte_async_tcf0();
> 
> 
> 
> >  }
> >  
> >  asmlinkage void noinstr exit_to_user_mode(void)
> >  {
> > +	/* Ignore asynchronous tag check faults in the uaccess routines */
> > +	clear_mte_async_tcf0();
> > +
> >  	trace_hardirqs_on_prepare();
> >  	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
> >  	user_enter_irqoff();
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index a31a0a713c85..fb57df0d453f 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -34,15 +34,11 @@
> >   * user and kernel mode.
> >   */
> >  	.macro user_exit_irqoff
> > -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
> >  	bl	enter_from_user_mode
> > -#endif
> >  	.endm
> >  
> >  	.macro user_enter_irqoff
> > -#if defined(CONFIG_CONTEXT_TRACKING) || defined(CONFIG_TRACE_IRQFLAGS)
> >  	bl	exit_to_user_mode
> > -#endif
> >  	.endm
> >  
> >  	.macro	clear_gp_regs
> > @@ -147,32 +143,6 @@ alternative_cb_end
> >  .L__asm_ssbd_skip\@:
> >  	.endm
> >  
> > -	/* Check for MTE asynchronous tag check faults */
> > -	.macro check_mte_async_tcf, flgs, tmp
> > -#ifdef CONFIG_ARM64_MTE
> > -alternative_if_not ARM64_MTE
> > -	b	1f
> > -alternative_else_nop_endif
> > -	mrs_s	\tmp, SYS_TFSRE0_EL1
> > -	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
> > -	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
> > -	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
> > -	str	\flgs, [tsk, #TSK_TI_FLAGS]
> > -	msr_s	SYS_TFSRE0_EL1, xzr
> > -1:
> > -#endif
> > -	.endm
> > -
> > -	/* Clear the MTE asynchronous tag check faults */
> > -	.macro clear_mte_async_tcf
> > -#ifdef CONFIG_ARM64_MTE
> > -alternative_if ARM64_MTE
> > -	dsb	ish
> > -	msr_s	SYS_TFSRE0_EL1, xzr
> > -alternative_else_nop_endif
> > -#endif
> > -	.endm
> > -
> >  	.macro mte_set_gcr, tmp, tmp2
> >  #ifdef CONFIG_ARM64_MTE
> >  	/*
> > @@ -243,8 +213,6 @@ alternative_else_nop_endif
> >  	ldr	x19, [tsk, #TSK_TI_FLAGS]
> >  	disable_step_tsk x19, x20
> >  
> > -	/* Check for asynchronous tag check faults in user space */
> > -	check_mte_async_tcf x19, x22
> >  	apply_ssbd 1, x22, x23
> >  
> >  	ptrauth_keys_install_kernel tsk, x20, x22, x23
> > @@ -775,8 +743,6 @@ SYM_CODE_START_LOCAL(ret_to_user)
> >  	cbnz	x2, work_pending
> >  finish_ret_to_user:
> >  	user_enter_irqoff
> > -	/* Ignore asynchronous tag check faults in the uaccess routines */
> > -	clear_mte_async_tcf
> >  	enable_step_tsk x19, x2
> >  #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> >  	bl	stackleak_erase
> > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index b3c70a612c7a..84a942c25870 100644
> > --- a/arch/arm64/kernel/mte.c
> > +++ b/arch/arm64/kernel/mte.c
> > @@ -166,14 +166,43 @@ static void set_gcr_el1_excl(u64 excl)
> >  	 */
> >  }
> >  
> > -void flush_mte_state(void)
> > +void noinstr check_mte_async_tcf0(void)
> > +{
> > +	u64 tcf0;
> > +
> > +	if (!system_supports_mte())
> > +		return;
> > +
> > +	/*
> > +	 * dsb(ish) is not required before the register read
> > +	 * because the TFSRE0_EL1 is automatically synchronized
> > +	 * by the hardware on exception entry as SCTLR_EL1.ITFSB
> > +	 * is set.
> > +	 */
> > +	tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
> > +
> > +	if (tcf0 & SYS_TFSR_EL1_TF0)
> > +		set_thread_flag(TIF_MTE_ASYNC_FAULT);
> > +
> > +	write_sysreg_s(0, SYS_TFSRE0_EL1);
> > +}
> > +
> > +void noinstr clear_mte_async_tcf0(void)
> >  {
> >  	if (!system_supports_mte())
> >  		return;
> >  
> > -	/* clear any pending asynchronous tag fault */
> >  	dsb(ish);
> >  	write_sysreg_s(0, SYS_TFSRE0_EL1);
> > +}
> > +
> > +void flush_mte_state(void)
> > +{
> > +	if (!system_supports_mte())
> > +		return;
> > +
> > +	/* clear any pending asynchronous tag fault */
> > +	clear_mte_async_tcf0();
> >  	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
> >  	/* disable tag checking */
> >  	set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
> > -- 
> > 2.30.2
> > 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
  2021-04-09 16:18     ` Mark Rutland
@ 2021-04-09 16:56       ` Catalin Marinas
  -1 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2021-04-09 16:56 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Vincenzo Frascino, linux-arm-kernel, linux-kernel, kasan-dev,
	Will Deacon, stable

On Fri, Apr 09, 2021 at 05:18:45PM +0100, Mark Rutland wrote:
> On Fri, Apr 09, 2021 at 03:32:47PM +0100, Mark Rutland wrote:
> > Hi Vincenzo,
> > 
> > On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> > > The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
> > > race with another CPU doing a set_tsk_thread_flag() and all the other flags
> > > can be lost in the process.
> > > 
> > > Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
> > > exit_to_user_mode() to address the problem.
> > > 
> > > Note: Moving the check in entry-common allows to use set_thread_flag()
> > > which is safe.
> 
> I've dug into this a bit more, and as set_thread_flag() calls some
> potentially-instrumented helpers I don't think this is safe after all
> (as e.g. those might cause an EL1 exception and clobber the ESR/FAR/etc
> before the EL0 exception handler reads it).
> 
> Making that watertight is pretty hairy, as we either need to open-code
> set_thread_flag() or go rework a load of core code. If we can use STSET
> in the entry asm that'd be simpler, otherwise we'll need something more
> involved.

I hacked this up quickly:

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 9b4d629f7628..25efe83d68a4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1646,6 +1646,7 @@ config ARM64_AS_HAS_MTE
 config ARM64_MTE
 	bool "Memory Tagging Extension support"
 	default y
+	depends on ARM64_LSE_ATOMICS
 	depends on ARM64_AS_HAS_MTE && ARM64_TAGGED_ADDR_ABI
 	depends on AS_HAS_ARMV8_5
 	# Required for tag checking in the uaccess routines
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index a45b4ebbfe7d..ad29892f2974 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -148,16 +148,18 @@ alternative_cb_end
 	.endm
 
 	/* Check for MTE asynchronous tag check faults */
-	.macro check_mte_async_tcf, flgs, tmp
+	.macro check_mte_async_tcf, tmp, ti_flags
 #ifdef CONFIG_ARM64_MTE
+	.arch_extension lse
 alternative_if_not ARM64_MTE
 	b	1f
 alternative_else_nop_endif
 	mrs_s	\tmp, SYS_TFSRE0_EL1
 	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
 	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
-	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
-	str	\flgs, [tsk, #TSK_TI_FLAGS]
+	mov	\tmp, #_TIF_MTE_ASYNC_FAULT
+	add	\ti_flags, tsk, #TSK_TI_FLAGS
+	stset	\tmp, [\ti_flags]
 	msr_s	SYS_TFSRE0_EL1, xzr
 1:
 #endif
@@ -244,7 +246,7 @@ alternative_else_nop_endif
 	disable_step_tsk x19, x20
 
 	/* Check for asynchronous tag check faults in user space */
-	check_mte_async_tcf x19, x22
+	check_mte_async_tcf x22, x23
 	apply_ssbd 1, x22, x23
 
 	ptrauth_keys_install_kernel tsk, x20, x22, x23

-- 
Catalin

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common
@ 2021-04-09 16:56       ` Catalin Marinas
  0 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2021-04-09 16:56 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Vincenzo Frascino, linux-arm-kernel, linux-kernel, kasan-dev,
	Will Deacon, stable

On Fri, Apr 09, 2021 at 05:18:45PM +0100, Mark Rutland wrote:
> On Fri, Apr 09, 2021 at 03:32:47PM +0100, Mark Rutland wrote:
> > Hi Vincenzo,
> > 
> > On Fri, Apr 09, 2021 at 02:24:19PM +0100, Vincenzo Frascino wrote:
> > > The check_mte_async_tcf macro sets the TIF flag non-atomically. This can
> > > race with another CPU doing a set_tsk_thread_flag() and all the other flags
> > > can be lost in the process.
> > > 
> > > Move the tcf0 check to enter_from_user_mode() and clear tcf0 in
> > > exit_to_user_mode() to address the problem.
> > > 
> > > Note: Moving the check in entry-common allows to use set_thread_flag()
> > > which is safe.
> 
> I've dug into this a bit more, and as set_thread_flag() calls some
> potentially-instrumented helpers I don't think this is safe after all
> (as e.g. those might cause an EL1 exception and clobber the ESR/FAR/etc
> before the EL0 exception handler reads it).
> 
> Making that watertight is pretty hairy, as we either need to open-code
> set_thread_flag() or go rework a load of core code. If we can use STSET
> in the entry asm that'd be simpler, otherwise we'll need something more
> involved.

I hacked this up quickly:

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 9b4d629f7628..25efe83d68a4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1646,6 +1646,7 @@ config ARM64_AS_HAS_MTE
 config ARM64_MTE
 	bool "Memory Tagging Extension support"
 	default y
+	depends on ARM64_LSE_ATOMICS
 	depends on ARM64_AS_HAS_MTE && ARM64_TAGGED_ADDR_ABI
 	depends on AS_HAS_ARMV8_5
 	# Required for tag checking in the uaccess routines
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index a45b4ebbfe7d..ad29892f2974 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -148,16 +148,18 @@ alternative_cb_end
 	.endm
 
 	/* Check for MTE asynchronous tag check faults */
-	.macro check_mte_async_tcf, flgs, tmp
+	.macro check_mte_async_tcf, tmp, ti_flags
 #ifdef CONFIG_ARM64_MTE
+	.arch_extension lse
 alternative_if_not ARM64_MTE
 	b	1f
 alternative_else_nop_endif
 	mrs_s	\tmp, SYS_TFSRE0_EL1
 	tbz	\tmp, #SYS_TFSR_EL1_TF0_SHIFT, 1f
 	/* Asynchronous TCF occurred for TTBR0 access, set the TI flag */
-	orr	\flgs, \flgs, #_TIF_MTE_ASYNC_FAULT
-	str	\flgs, [tsk, #TSK_TI_FLAGS]
+	mov	\tmp, #_TIF_MTE_ASYNC_FAULT
+	add	\ti_flags, tsk, #TSK_TI_FLAGS
+	stset	\tmp, [\ti_flags]
 	msr_s	SYS_TFSRE0_EL1, xzr
 1:
 #endif
@@ -244,7 +246,7 @@ alternative_else_nop_endif
 	disable_step_tsk x19, x20
 
 	/* Check for asynchronous tag check faults in user space */
-	check_mte_async_tcf x19, x22
+	check_mte_async_tcf x22, x23
 	apply_ssbd 1, x22, x23
 
 	ptrauth_keys_install_kernel tsk, x20, x22, x23

-- 
Catalin

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-04-09 16:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09 13:24 [PATCH v3] arm64: mte: Move MTE TCF0 check in entry-common Vincenzo Frascino
2021-04-09 13:24 ` Vincenzo Frascino
2021-04-09 14:32 ` Mark Rutland
2021-04-09 14:32   ` Mark Rutland
2021-04-09 16:18   ` Mark Rutland
2021-04-09 16:18     ` Mark Rutland
2021-04-09 16:56     ` Catalin Marinas
2021-04-09 16:56       ` Catalin Marinas
2021-04-09 14:47 ` Catalin Marinas
2021-04-09 14:47   ` Catalin Marinas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.