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 the flag 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 <[email protected]>
Cc: Will Deacon <[email protected]>
Reported-by: Will Deacon <[email protected]>
Signed-off-by: Vincenzo Frascino <[email protected]>
---
arch/arm64/include/asm/mte.h | 8 ++++++++
arch/arm64/kernel/entry-common.c | 6 ++++++
arch/arm64/kernel/entry.S | 30 ------------------------------
arch/arm64/kernel/mte.c | 25 +++++++++++++++++++++++--
4 files changed, 37 insertions(+), 32 deletions(-)
diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 9b557a457f24..188f778c6f7b 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -31,6 +31,8 @@ void mte_invalidate_tags(int type, pgoff_t offset);
void mte_invalidate_tags_area(int type);
void *mte_allocate_tag_storage(void);
void mte_free_tag_storage(char *storage);
+void check_mte_async_tcf0(void);
+void clear_mte_async_tcf0(void);
#ifdef CONFIG_ARM64_MTE
@@ -83,6 +85,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
{
return -EIO;
}
+void check_mte_async_tcf0(void)
+{
+}
+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..fafd74ae5021 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -147,32 +147,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 +217,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 +747,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..e759b0eca47e 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -166,14 +166,35 @@ static void set_gcr_el1_excl(u64 excl)
*/
}
+void check_mte_async_tcf0(void)
+{
+ /*
+ * 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.
+ */
+ u64 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 clear_mte_async_tcf0(void)
+{
+ 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 */
- dsb(ish);
- write_sysreg_s(0, SYS_TFSRE0_EL1);
+ 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
On Thu, Apr 08, 2021 at 03:37:23PM +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 the flag can be
> lost in the process.
Actually, it's all the *other* flags that get lost!
> 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 <[email protected]>
> Cc: Will Deacon <[email protected]>
> Reported-by: Will Deacon <[email protected]>
> Signed-off-by: Vincenzo Frascino <[email protected]>
> ---
> arch/arm64/include/asm/mte.h | 8 ++++++++
> arch/arm64/kernel/entry-common.c | 6 ++++++
> arch/arm64/kernel/entry.S | 30 ------------------------------
> arch/arm64/kernel/mte.c | 25 +++++++++++++++++++++++--
> 4 files changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 9b557a457f24..188f778c6f7b 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -31,6 +31,8 @@ void mte_invalidate_tags(int type, pgoff_t offset);
> void mte_invalidate_tags_area(int type);
> void *mte_allocate_tag_storage(void);
> void mte_free_tag_storage(char *storage);
> +void check_mte_async_tcf0(void);
> +void clear_mte_async_tcf0(void);
>
> #ifdef CONFIG_ARM64_MTE
>
> @@ -83,6 +85,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
> {
> return -EIO;
> }
> +void check_mte_async_tcf0(void)
> +{
> +}
> +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();
> }
Is enter_from_user_mode() always called when we enter the kernel from EL0?
afaict, some paths (e.g. el0_irq()) only end up calling it if
CONTEXT_TRACKING or TRACE_IRQFLAGS are enabled.
>
> asmlinkage void noinstr exit_to_user_mode(void)
> {
> + /* Ignore asynchronous tag check faults in the uaccess routines */
> + clear_mte_async_tcf0();
> +
and this one seems to be called even less often.
Will
On Thu, Apr 08, 2021 at 03:56:04PM +0100, Will Deacon wrote:
> On Thu, Apr 08, 2021 at 03:37:23PM +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 the flag can be
> > lost in the process.
>
> Actually, it's all the *other* flags that get lost!
>
> > 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 <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Reported-by: Will Deacon <[email protected]>
> > Signed-off-by: Vincenzo Frascino <[email protected]>
> > ---
> > arch/arm64/include/asm/mte.h | 8 ++++++++
> > arch/arm64/kernel/entry-common.c | 6 ++++++
> > arch/arm64/kernel/entry.S | 30 ------------------------------
> > arch/arm64/kernel/mte.c | 25 +++++++++++++++++++++++--
> > 4 files changed, 37 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> > index 9b557a457f24..188f778c6f7b 100644
> > --- a/arch/arm64/include/asm/mte.h
> > +++ b/arch/arm64/include/asm/mte.h
> > @@ -31,6 +31,8 @@ void mte_invalidate_tags(int type, pgoff_t offset);
> > void mte_invalidate_tags_area(int type);
> > void *mte_allocate_tag_storage(void);
> > void mte_free_tag_storage(char *storage);
> > +void check_mte_async_tcf0(void);
> > +void clear_mte_async_tcf0(void);
> >
> > #ifdef CONFIG_ARM64_MTE
> >
> > @@ -83,6 +85,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
> > {
> > return -EIO;
> > }
> > +void check_mte_async_tcf0(void)
> > +{
> > +}
> > +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();
> > }
>
> Is enter_from_user_mode() always called when we enter the kernel from EL0?
> afaict, some paths (e.g. el0_irq()) only end up calling it if
> CONTEXT_TRACKING or TRACE_IRQFLAGS are enabled.
Currently everything that's in {enter,exit}_from_user_mode() only
matters when either CONTEXT_TRACKING or TRACE_IRQFLAGS is selected (and
expands to an empty stub otherwise).
We could drop the ifdeffery in user_{enter,exit}_irqoff() to have them
called regardless, or add CONFIG_MTE to the list.
> > asmlinkage void noinstr exit_to_user_mode(void)
> > {
> > + /* Ignore asynchronous tag check faults in the uaccess routines */
> > + clear_mte_async_tcf0();
> > +
>
> and this one seems to be called even less often.
This is always done in ret_to_user, so (modulo ifdeferry above) all
returns to EL0 call this.
Thanks,
Mark.
Hi Vincenzo,
On Thu, Apr 08, 2021 at 03:37:23PM +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 the flag 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.
Beware that these are called at critical points of the entry sequence,
so we need to take care that nothing is instrumented (e.g. we can only
safely use noinstr functions here).
> 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 <[email protected]>
> Cc: Will Deacon <[email protected]>
> Reported-by: Will Deacon <[email protected]>
> Signed-off-by: Vincenzo Frascino <[email protected]>
> ---
> arch/arm64/include/asm/mte.h | 8 ++++++++
> arch/arm64/kernel/entry-common.c | 6 ++++++
> arch/arm64/kernel/entry.S | 30 ------------------------------
> arch/arm64/kernel/mte.c | 25 +++++++++++++++++++++++--
> 4 files changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 9b557a457f24..188f778c6f7b 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -31,6 +31,8 @@ void mte_invalidate_tags(int type, pgoff_t offset);
> void mte_invalidate_tags_area(int type);
> void *mte_allocate_tag_storage(void);
> void mte_free_tag_storage(char *storage);
> +void check_mte_async_tcf0(void);
> +void clear_mte_async_tcf0(void);
>
> #ifdef CONFIG_ARM64_MTE
>
> @@ -83,6 +85,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
> {
> return -EIO;
> }
> +void check_mte_async_tcf0(void)
> +{
> +}
> +void clear_mte_async_tcf0(void)
> +{
> +}
Were these meant to be static inline?
> 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..fafd74ae5021 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -147,32 +147,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 +217,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 +747,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..e759b0eca47e 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -166,14 +166,35 @@ static void set_gcr_el1_excl(u64 excl)
> */
> }
>
> +void check_mte_async_tcf0(void)
As above, this'll need to be noinstr. I also reckon we should put this
in the header so that it can be inlined.
> +{
> + /*
> + * 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.
> + */
> + u64 tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
Shouldn't we have an MTE feature check first?
> +
> + if (tcf0 & SYS_TFSR_EL1_TF0)
> + set_thread_flag(TIF_MTE_ASYNC_FAULT);
> +
> + write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}
> +
> +void clear_mte_async_tcf0(void)
> +{
> + dsb(ish);
> + write_sysreg_s(0, SYS_TFSRE0_EL1);
> +}
Likewise here on all counts.
Thanks,
Mark.
> void flush_mte_state(void)
> {
> if (!system_supports_mte())
> return;
>
> /* clear any pending asynchronous tag fault */
> - dsb(ish);
> - write_sysreg_s(0, SYS_TFSRE0_EL1);
> + 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
>
On 4/8/21 4:06 PM, Mark Rutland wrote:
> On Thu, Apr 08, 2021 at 03:56:04PM +0100, Will Deacon wrote:
>> On Thu, Apr 08, 2021 at 03:37:23PM +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 the flag can be
>>> lost in the process.
>>
>> Actually, it's all the *other* flags that get lost!
>>
You are right, I need to explain this better.
...
--
Regards,
Vincenzo
On 4/8/21 4:18 PM, Mark Rutland wrote:
> Hi Vincenzo,
>
> On Thu, Apr 08, 2021 at 03:37:23PM +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 the flag 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.
>
> Beware that these are called at critical points of the entry sequence,
> so we need to take care that nothing is instrumented (e.g. we can only
> safely use noinstr functions here).
>
Sure, I will add noinstr in the next version of the patch.
>> 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 <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Reported-by: Will Deacon <[email protected]>
>> Signed-off-by: Vincenzo Frascino <[email protected]>
>> ---
>> arch/arm64/include/asm/mte.h | 8 ++++++++
>> arch/arm64/kernel/entry-common.c | 6 ++++++
>> arch/arm64/kernel/entry.S | 30 ------------------------------
>> arch/arm64/kernel/mte.c | 25 +++++++++++++++++++++++--
>> 4 files changed, 37 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
>> index 9b557a457f24..188f778c6f7b 100644
>> --- a/arch/arm64/include/asm/mte.h
>> +++ b/arch/arm64/include/asm/mte.h
>> @@ -31,6 +31,8 @@ void mte_invalidate_tags(int type, pgoff_t offset);
>> void mte_invalidate_tags_area(int type);
>> void *mte_allocate_tag_storage(void);
>> void mte_free_tag_storage(char *storage);
>> +void check_mte_async_tcf0(void);
>> +void clear_mte_async_tcf0(void);
>>
>> #ifdef CONFIG_ARM64_MTE
>>
>> @@ -83,6 +85,12 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
>> {
>> return -EIO;
>> }
>> +void check_mte_async_tcf0(void)
>> +{
>> +}
>> +void clear_mte_async_tcf0(void)
>> +{
>> +}
>
> Were these meant to be static inline?
>
Agree, it definitely needs static inline here.
>> 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..fafd74ae5021 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -147,32 +147,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 +217,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 +747,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..e759b0eca47e 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -166,14 +166,35 @@ static void set_gcr_el1_excl(u64 excl)
>> */
>> }
>>
>> +void check_mte_async_tcf0(void)
>
> As above, this'll need to be noinstr. I also reckon we should put this
> in the header so that it can be inlined.
>
Yes, I agree.
>> +{
>> + /*
>> + * 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.
>> + */
>> + u64 tcf0 = read_sysreg_s(SYS_TFSRE0_EL1);
>
> Shouldn't we have an MTE feature check first?
>
Indeed, I will add it in the next version.
>> +
>> + if (tcf0 & SYS_TFSR_EL1_TF0)
>> + set_thread_flag(TIF_MTE_ASYNC_FAULT);
>> +
>> + write_sysreg_s(0, SYS_TFSRE0_EL1);
>> +}
>> +
>> +void clear_mte_async_tcf0(void)
>> +{
>> + dsb(ish);
>> + write_sysreg_s(0, SYS_TFSRE0_EL1);
>> +}
>
> Likewise here on all counts.
>
I will add noinstr and the check in the next version.
> Thanks,
> Mark.
>
>> void flush_mte_state(void)
>> {
>> if (!system_supports_mte())
>> return;
>>
>> /* clear any pending asynchronous tag fault */
>> - dsb(ish);
>> - write_sysreg_s(0, SYS_TFSRE0_EL1);
>> + 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
>>
--
Regards,
Vincenzo
On Thu, Apr 08, 2021 at 04:06:23PM +0100, Mark Rutland wrote:
> On Thu, Apr 08, 2021 at 03:56:04PM +0100, Will Deacon wrote:
> > On Thu, Apr 08, 2021 at 03:37:23PM +0100, Vincenzo Frascino wrote:
> > > 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();
> > > }
> >
> > Is enter_from_user_mode() always called when we enter the kernel from EL0?
> > afaict, some paths (e.g. el0_irq()) only end up calling it if
> > CONTEXT_TRACKING or TRACE_IRQFLAGS are enabled.
>
> Currently everything that's in {enter,exit}_from_user_mode() only
> matters when either CONTEXT_TRACKING or TRACE_IRQFLAGS is selected (and
> expands to an empty stub otherwise).
>
> We could drop the ifdeffery in user_{enter,exit}_irqoff() to have them
> called regardless, or add CONFIG_MTE to the list.
I'm always in favour of dropping ifdeffery if it's getting in the way.
> > > asmlinkage void noinstr exit_to_user_mode(void)
> > > {
> > > + /* Ignore asynchronous tag check faults in the uaccess routines */
> > > + clear_mte_async_tcf0();
> > > +
> >
> > and this one seems to be called even less often.
>
> This is always done in ret_to_user, so (modulo ifdeferry above) all
> returns to EL0 call this.
Right, I was just saying that if you disabled those CONFIG options then this
isn't called _at all_ whereas I think enter_from_user_mode() still is on
some paths.
Will