| Message ID | 17ec8af55dc0a4d3ade679feb0858f0df4c80d27.1600987622.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: add hardware tag-based mode for arm64 | expand |
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.
On Fri, Sep 25, 2020 at 12:49 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > 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. I did, see the last line of the commit description. > > + > > 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. The fault can happen on multiple cores at the same time, right? In that case without READ/WRITE_ONCE() we'll have a data-race here.
On Fri, Sep 25, 2020 at 01:26:02PM +0200, Andrey Konovalov wrote: > On Fri, Sep 25, 2020 at 12:49 PM Catalin Marinas > <catalin.marinas@arm.com> wrote: > > > 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. > > I did, see the last line of the commit description. Sorry, I missed that. > > > + > > > 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. > > The fault can happen on multiple cores at the same time, right? In > that case without READ/WRITE_ONCE() we'll have a data-race here. READ/WRITE_ONCE won't magically solve such races. If two CPUs enter simultaneously in do_tag_recovery(), they'd both read 'reported' as false and both print the fault info. If you really care about this race, you need to atomically both read and update the variable with an xchg() or cmpxchg().
On Fri, Sep 25, 2020 at 1:47 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Fri, Sep 25, 2020 at 01:26:02PM +0200, Andrey Konovalov wrote: > > On Fri, Sep 25, 2020 at 12:49 PM Catalin Marinas > > <catalin.marinas@arm.com> wrote: > > > > 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. > > > > I did, see the last line of the commit description. > > Sorry, I missed that. No problem! > > > > + > > > > 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. > > > > The fault can happen on multiple cores at the same time, right? In > > that case without READ/WRITE_ONCE() we'll have a data-race here. > > READ/WRITE_ONCE won't magically solve such races. If two CPUs enter > simultaneously in do_tag_recovery(), they'd both read 'reported' as > false and both print the fault info. They won't solve the race condition, but they will solve the data race. I guess here we don't really care about the race condition, as printing a tag fault twice is OK. But having a data race here will lead to KCSAN reports, although won't probably break anything in practice. > If you really care about this race, you need to atomically both read and > update the variable with an xchg() or cmpxchg().
On Fri, Sep 25, 2020 at 01:52:56PM +0200, Andrey Konovalov wrote: > On Fri, Sep 25, 2020 at 1:47 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Fri, Sep 25, 2020 at 01:26:02PM +0200, Andrey Konovalov wrote: > > > On Fri, Sep 25, 2020 at 12:49 PM Catalin Marinas > > > <catalin.marinas@arm.com> wrote: > > > > > + > > > > > 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. > > > > > > The fault can happen on multiple cores at the same time, right? In > > > that case without READ/WRITE_ONCE() we'll have a data-race here. > > > > READ/WRITE_ONCE won't magically solve such races. If two CPUs enter > > simultaneously in do_tag_recovery(), they'd both read 'reported' as > > false and both print the fault info. > > They won't solve the race condition, but they will solve the data > race. I guess here we don't really care about the race condition, as > printing a tag fault twice is OK. But having a data race here will > lead to KCSAN reports, although won't probably break anything in > practice. I agree, in practice it should be fine. Anyway, it doesn't hurt leaving them in place.
On Fri, Sep 25, 2020 at 12:50:33AM +0200, Andrey Konovalov wrote: > From: Vincenzo Frascino <vincenzo.frascino@arm.com> > > Add the implementation of the in-kernel fault handler. > > When a tag fault happens on a kernel address: > * MTE is disabled on the current CPU, > * the execution continues. > > When a tag fault happens on a user address: > * the kernel executes do_bad_area() and panics. > > The tag fault handler for kernel addresses is currently empty and will be > filled in by a future commit. > > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > Co-developed-by: Andrey Konovalov <andreyknvl@google.com> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
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); } 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) +{ +} + 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); + } + + /* + * Disable MTE Tag Checking on the local CPU for the current EL. + * It will be done lazily on the other CPUs when they will hit a + * tag fault. + */ + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_NONE); + isb(); +} + + static int do_tag_check_fault(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - do_bad_area(addr, esr, regs); + /* + * The tag check fault (TCF) is per EL, hence TCF0 affects + * EL0 and TCF affects EL1. + * TTBR0 address belong by convention to EL0 hence to correctly + * discriminate we use the is_ttbr0_addr() macro. + */ + if (is_ttbr0_addr(addr)) + do_bad_area(addr, esr, regs); + else + do_tag_recovery(addr, esr, regs); + return 0; }