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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200925104933.GD4846@gaia>
Date:   Fri, 25 Sep 2020 11:49:33 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        kasan-dev@...glegroups.com,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Marco Elver <elver@...gle.com>,
        Evgenii Stepanov <eugenis@...gle.com>,
        Elena Petrova <lenaptr@...gle.com>,
        Branislav Rankov <Branislav.Rankov@....com>,
        Kevin Brodsky <kevin.brodsky@....com>,
        Will Deacon <will.deacon@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-arm-kernel@...ts.infradead.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 26/39] arm64: mte: Add in-kernel tag fault handler

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