linux-kernel - Re: [PATCH v3 05/12] arm64: csum: Disable KASAN for do

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]
Date:   Thu, 16 Apr 2020 12:53:46 +0100
From:   Will Deacon <will@...nel.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        kernel-team@...roid.com, Michael Ellerman <mpe@...erman.id.au>,
        Peter Zijlstra <peterz@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH v3 05/12] arm64: csum: Disable KASAN for do_csum()

On Thu, Apr 16, 2020 at 10:31:06AM +0100, Mark Rutland wrote:
> On Wed, Apr 15, 2020 at 08:26:05PM +0100, Will Deacon wrote:
> > I think abusing a relaxed concurrency primitive for this is
> > not the right thing to do, particularly when the __no_sanitize_address
> > annotation is available. I fact, it's almost an argument in favour
> > of removing READ_ONCE_NOCHECK() so that people use the annotation instead!
> 
> Arguably we *are* using it as a relaxed concurrency primitive, to get a
> snapshot of a varaible undergoing concurrent modification.

That's fair, so it's only the checksum code that was abusing this, which
I've fixed.

> FWIW, for the arm64 unwind code we could add a helper to snapshot the
> frame record, and mark that as __no_sanitize_address, e.g.
> 
> /*
>  * Get a snapshot of a frame record that might be undergoing concurrent
>  * modification (and hence we must also avoid a KASAN splat).
>  */
> static __no_sanitize_address snapshot_frame(struct stackframe *frame,
> 					    unsigned long fp)
> {
> 	frame->fp = READ_ONCE(*(unsigned long *)(fp));
> 	frame->pc = READ_ONCE(*(unsigned long *)(fp + 8));
> }
> 
> ... we'd need to do likewied in a few bits of unwind code:
> 
> arch/s390/kernel/unwind_bc.c:	       READ_ONCE_NOCHECK(regs->psw.mask) & PSW_MASK_PSTATE;
> arch/s390/kernel/unwind_bc.c:		ip = READ_ONCE_NOCHECK(sf->gprs[8]);
> arch/s390/kernel/unwind_bc.c:		sp = READ_ONCE_NOCHECK(sf->back_chain);
> arch/s390/kernel/unwind_bc.c:			ip = READ_ONCE_NOCHECK(sf->gprs[8]);
> arch/s390/kernel/unwind_bc.c:			ip = READ_ONCE_NOCHECK(regs->psw.addr);
> arch/s390/kernel/unwind_bc.c:			sp = READ_ONCE_NOCHECK(regs->gprs[15]);
> arch/s390/kernel/unwind_bc.c:		ip = READ_ONCE_NOCHECK(sf->gprs[8]);
> arch/x86/include/asm/atomic.h:	 * Note for KASAN: we deliberately don't use READ_ONCE_NOCHECK() here,
> arch/x86/include/asm/unwind.h:		val = READ_ONCE_NOCHECK(x);		\
> arch/x86/kernel/dumpstack.c:			unsigned long addr = READ_ONCE_NOCHECK(*stack);
> arch/x86/kernel/process.c:	fp = READ_ONCE_NOCHECK(((struct inactive_task_frame *)sp)->bp);
> arch/x86/kernel/process.c:		ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
> arch/x86/kernel/process.c:		fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
> arch/x86/kernel/unwind_frame.c:			word = READ_ONCE_NOCHECK(*sp);
> arch/x86/kernel/unwind_guess.c:	addr = READ_ONCE_NOCHECK(*state->sp);
> arch/x86/kernel/unwind_guess.c:			unsigned long addr = READ_ONCE_NOCHECK(*state->sp);
> arch/x86/kernel/unwind_orc.c:	*val = READ_ONCE_NOCHECK(*(unsigned long *)addr);
> arch/x86/kernel/unwind_orc.c:		state->bp = READ_ONCE_NOCHECK(frame->bp);
> arch/x86/kernel/unwind_orc.c:		state->ip = READ_ONCE_NOCHECK(frame->ret_addr);
> include/linux/compiler.h: * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need
> include/linux/compiler.h:#define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
> kernel/trace/trace_stack.c:			 * The READ_ONCE_NOCHECK is used to let KASAN know that
> kernel/trace/trace_stack.c:			if ((READ_ONCE_NOCHECK(*p)) == stack_dump_trace[i]) {

Indeed. For now, I'm going to keep this simple with the change below, but
I'll revisit this later on because I have another series removing
smp_read_barrier_depends() which makes this a lot simpler.

Will

--->8

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 00a68063d9d5..c363d8debc43 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -212,18 +212,12 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	(typeof(x))__x;							\
 })
 
-/*
- * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need
- * to hide memory access from KASAN.
- */
-#define READ_ONCE_NOCHECK(x)						\
+#define READ_ONCE(x)							\
 ({									\
 	compiletime_assert_rwonce_type(x);				\
 	__READ_ONCE_SCALAR(x);						\
 })
 
-#define READ_ONCE(x)	READ_ONCE_NOCHECK(x)
-
 #define __WRITE_ONCE(x, val)				\
 do {							\
 	*(volatile typeof(x) *)&(x) = (val);		\
@@ -247,6 +241,24 @@ do {							\
 # define __no_kasan_or_inline __always_inline
 #endif
 
+static __no_kasan_or_inline
+unsigned long __read_once_word_nocheck(const void *addr)
+{
+	return __READ_ONCE(*(unsigned long *)addr);
+}
+
+/*
+ * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need to load a
+ * word from memory atomically but without telling KASAN. This is usually
+ * used by unwinding code when walking the stack of a running process.
+ */
+#define READ_ONCE_NOCHECK(x)						\
+({									\
+	unsigned long __x = __read_once_word_nocheck(&(x));		\
+	smp_read_barrier_depends();					\
+	__x;								\
+})
+
 static __no_kasan_or_inline
 unsigned long read_word_at_a_time(const void *addr)
 {