lkml.org 
[lkml]   [2020]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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)
    {
    \
     
     \ /
      Last update: 2020-04-16 13:56    [W:4.874 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site