Re: [PATCH] kasan: fix kasan_check_read/write definitions

[Dmitry Vyukov <dvyukov@google.com>]

On Tue, Dec 11, 2018 at 11:25 PM Alexander Potapenko <glider@google.com> wrote:
> > > Building little-endian allmodconfig kernels on arm64 started failing
> > > with the generated atomic.h implementation, since we now try to call
> > > kasan helpers from the EFI stub:
> > >
> > > aarch64-linux-gnu-ld: drivers/firmware/efi/libstub/arm-stub.stub.o: in function `atomic_set':
> > > include/generated/atomic-instrumented.h:44: undefined reference to `__efistub_kasan_check_write'
> > >
> > > I suspect that we get similar problems in other files that explicitly
> > > disable KASAN for some reason but call atomic_t based helper functions.
> > >
> > > We can fix this by checking the predefined __SANITIZE_ADDRESS__ macro
> > > that the compiler sets instead of checking CONFIG_KASAN, but this in turn
> > > requires a small hack in mm/kasan/common.c so we do see the extern
> > > declaration there instead of the inline function.
> >
> >
> > Alexander, I think you are doing a similar thing for similar reasons
> > in KMSAN patch (see KMSAN_CHECK_ATOMIC_PARAMS):
> > https://github.com/google/kmsan/commit/17ebbfe19624c84adf79b0e5a74fd258c49ff12b
> > Namely, non-KMSAN-instrumented files must not get KMSAN callbacks from
> > atomics too.
> I'll need to double-check, but it occurs to me that we won't need
> additional hooks for atomics in KMSAN - the compiler instrumentation
> should suffice.

Compiler asm instrumentation will only insert conservative
initialization, but not checks of arguments, right?
I mean these checks are optional in the sense that it's only false
negatives, but since we already have them and they don't seem to lead
to false positives, why do we want to remove them?


> > Arnd patch does it the other way around: non-instrumented files need
> > to opt-in instead of opt-out.
> Shouldn't we put __SANITIZE_ADDRESS__ somewhere into mm/kasan/kasan.h then?
> > Let's settle on a common way to do this, so that we can use it
> > consistently across all tools.
> >
> >
> >
> > > Fixes: b1864b828644 ("locking/atomics: build atomic headers as required")
> > > Reported-by: Anders Roxell <anders.roxell@linaro.org>
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >  include/linux/kasan-checks.h | 2 +-
> > >  mm/kasan/common.c            | 2 ++
> > >  2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/kasan-checks.h b/include/linux/kasan-checks.h
> > > index d314150658a4..a61dc075e2ce 100644
> > > --- a/include/linux/kasan-checks.h
> > > +++ b/include/linux/kasan-checks.h
> > > @@ -2,7 +2,7 @@
> > >  #ifndef _LINUX_KASAN_CHECKS_H
> > >  #define _LINUX_KASAN_CHECKS_H
> > >
> > > -#ifdef CONFIG_KASAN
> > > +#if defined(__SANITIZE_ADDRESS__) || defined(__KASAN_INTERNAL)
> > >  void kasan_check_read(const volatile void *p, unsigned int size);
> > >  void kasan_check_write(const volatile void *p, unsigned int size);
> > >  #else
> > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > index 03d5d1374ca7..51a7932c33a3 100644
> > > --- a/mm/kasan/common.c
> > > +++ b/mm/kasan/common.c
> > > @@ -14,6 +14,8 @@
> > >   *
> > >   */
> > >
> > > +#define __KASAN_INTERNAL
> > > +
> > >  #include <linux/export.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/init.h>
> > > --
> > > 2.20.0
> > >
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg