Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov <dvyukov@google.com>]

On Thu, Feb 28, 2019 at 4:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> > On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > > be called from there, move it into an exception much like BUG/WARN.
> > >
> > > *compile tested only*
> >
> >
> > Please test it by booting KASAN kernel and then loading module
> > produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> > rely on "compile tested only", reviewers can't catch all of them
> > either.
>
> Sure, I'll do that. I just wanted to share the rest of the patches.
>
> A quick test shows it dies _REAAAAAAAALY_ early, as in:
>
> "Booting the kernel."
>
> is the first and very last thing it says... I wonder how I did that :-)
>
> > > +static __always_inline void
> > > +kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
> > > +{
> > > +       unsigned long rdi = addr, rsi = size, rdx = is_write, rcx = ip;
> > > +
> > > +       _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> > > +                  "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
> >
> > Can BUG return?
>
> Yes. Also see the annotate_reachable().
>
> > This should be able to return.
> > We also have other tools coming (KMSAN/KTSAN) where distinction
> > between fast path that does nothing and slower-paths are very blurred
> > and there are dozens of them, I don't think this BUG thunk will be
> > sustainable. What does BUG do what a normal call can't do?
>
> It keeps the SMAP validation rules nice and tight. If we were to add
> (and allow) things like pushf;clac;call ponies;popf or similar things,
> it all becomes complicated real quick.
>
> How would KMSAN/KTSAN interact with SMAP ?


I am missing some knowledge about SMAP to answer this.
In short, these tools insert lots of callbacks into runtime for memory
accesses, function entry/exit, atomicops and some other. These
callbacks can do things of different complexity.
Humm... perhaps we could just disable SMAP for KMSAN/KTSAN. It's
possible, right? If we have it enabled with KASAN, that should be
enough.

And the question from another thread: does SMAP detect bugs that KASAN
can't? If SMAP is not useful during stress testing/fuzzing, then we
could also disable it. But if it finds some bugs that KASAN won't
detect, then it would be pity to disable it.

Also, what's the actual problem with KASAN+SMAP? Is it warnings from
static analysis tool? Or there are also some runtime effects? What
effects?
Is it possible to disable the SMAP runtime checks once we enter
kasan_report() past report_enabled() check? We could restrict it to
"just finish printing this bug report whatever it takes and then
whatever" if it makes things simpler.
It would be nice if we could restrict it to something like:

@@ -291,6 +303,7 @@ void kasan_report(unsigned long addr, size_t size,
        if (likely(!report_enabled()))
                return;
+       disable_smap();

And then enforce panic at the end of report if smap is enabled.


> > > +       annotate_reachable();
> > > +}
> > > @@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka
> > >  EXPORT_SYMBOL(__asan_unregister_globals);
> > >
> > >  #define DEFINE_ASAN_LOAD_STORE(size)                                   \
> > > -       void __asan_load##size(unsigned long addr)                      \
> > > +       notrace void __asan_load##size(unsigned long addr)              \
> >
> >
> > We already have:
> > CFLAGS_REMOVE_generic.o = -pg
> > Doesn't it imply notrace for all functions?
>
> Indeed so, I'll make these hunks go away.