Re: [PATCH v3 26/39] arm64: mte: Add in-kernel tag fault handler

Catalin Marinas
2020-09-25T10:49:41Z

On Fri, Sep 25, 2020 at 12:50:33AM +0200, Andrey Konovalov wrote:
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 991dd5f031e4..c7fff8daf2a7 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -200,13 +200,36 @@ do { \
> CONFIG_ARM64_PAN)); \
> } while (0)
>
> +/*
> + * The Tag Check Flag (TCF) mode for MTE is per EL, hence TCF0
> + * affects EL0 and TCF affects EL1 irrespective of which TTBR is
> + * used.
> + * The kernel accesses TTBR0 usually with LDTR/STTR instructions
> + * when UAO is available, so these would act as EL0 accesses using
> + * TCF0.
> + * However futex.h code uses exclusives which would be executed as
> + * EL1, this can potentially cause a tag check fault even if the
> + * user disables TCF0.
> + *
> + * To address the problem we set the PSTATE.TCO bit in uaccess_enable()
> + * and reset it in uaccess_disable().
> + *
> + * The Tag check override (TCO) bit disables temporarily the tag checking
> + * preventing the issue.
> + */
> static inline void uaccess_disable(void)
> {
> + asm volatile(ALTERNATIVE("nop", SET_PSTATE_TCO(0),
> + ARM64_MTE, CONFIG_KASAN_HW_TAGS));
> +
> __uaccess_disable(ARM64_HAS_PAN);
> }
>
> static inline void uaccess_enable(void)
> {
> + asm volatile(ALTERNATIVE("nop", SET_PSTATE_TCO(1),
> + ARM64_MTE, CONFIG_KASAN_HW_TAGS));
> +
> __uaccess_enable(ARM64_HAS_PAN);
> }

This look fine.

> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index a3bd189602df..d110f382dacf 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -33,6 +33,7 @@
> #include <asm/debug-monitors.h>
> #include <asm/esr.h>
> #include <asm/kprobes.h>
> +#include <asm/mte.h>
> #include <asm/processor.h>
> #include <asm/sysreg.h>
> #include <asm/system_misc.h>
> @@ -294,6 +295,11 @@ static void die_kernel_fault(const char *msg, unsigned long addr,
> do_exit(SIGKILL);
> }
>
> +static void report_tag_fault(unsigned long addr, unsigned int esr,
> + struct pt_regs *regs)
> +{
> +}

Do we need to introduce report_tag_fault() in this patch? It's fine but
add a note in the commit log that it will be populated in a subsequent
patch.

> +
> static void __do_kernel_fault(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
> {
> @@ -641,10 +647,40 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> return 0;
> }
>
> +static void do_tag_recovery(unsigned long addr, unsigned int esr,
> + struct pt_regs *regs)
> +{
> + static bool reported = false;
> +
> + if (!READ_ONCE(reported)) {
> + report_tag_fault(addr, esr, regs);
> + WRITE_ONCE(reported, true);
> + }

I don't mind the READ_ONCE/WRITE_ONCE here but not sure what they help
with.

--
Catalin