Re: [PATCH v2 0/3] ubsan: Split out bounds checker

[Dmitry Vyukov]

On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> > On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > >
> > > v2:
> > >     - clarify Kconfig help text (aryabinin)
> > >     - add reviewed-by
> > >     - aim series at akpm, which seems to be where ubsan goes through?
> > > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@xxxxxxxxxxxx
> > >
> > > This splits out the bounds checker so it can be individually used. This
> > > is expected to be enabled in Android and hopefully for syzbot. Includes
> > > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> > >
> > > -Kees
> >
> > +syzkaller mailing list
> >
> > This is great!
>
> BTW, can I consider this your Acked-by for these patches? :)
>
> > I wanted to enable UBSAN on syzbot for a long time. And it's
> > _probably_ not lots of work. But it was stuck on somebody actually
> > dedicating some time specifically for it.
>
> Do you have a general mechanism to test that syzkaller will actually
> pick up the kernel log splat of a new check?

Yes. That's one of the most important and critical parts of syzkaller :)
The tests for different types of bugs are here:
https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report

But have 3 for UBSAN, but they may be old and it would be useful to
have 1 example crash per bug type:

syzkaller$ grep UBSAN pkg/report/testdata/linux/report/*
pkg/report/testdata/linux/report/40:TITLE: UBSAN: Undefined behaviour
in drivers/usb/core/devio.c:LINE
pkg/report/testdata/linux/report/40:[    4.556972] UBSAN: Undefined
behaviour in drivers/usb/core/devio.c:1517:25
pkg/report/testdata/linux/report/41:TITLE: UBSAN: Undefined behaviour
in ./arch/x86/include/asm/atomic.h:LINE
pkg/report/testdata/linux/report/41:[    3.805453] UBSAN: Undefined
behaviour in ./arch/x86/include/asm/atomic.h:156:2
pkg/report/testdata/linux/report/42:TITLE: UBSAN: Undefined behaviour
in kernel/time/hrtimer.c:LINE
pkg/report/testdata/linux/report/42:[   50.583499] UBSAN: Undefined
behaviour in kernel/time/hrtimer.c:310:16

One of them is incomplete and is parsed as "corrupted kernel output"
(won't be reported):
https://github.com/google/syzkaller/blob/master/pkg/report/testdata/linux/report/42

Also I see that report parsing just takes the first line, which
includes file name, which is suboptimal (too long, can't report 2 bugs
in the same file). We seem to converge on "bug-type in function-name"
format.
The thing about bug titles is that it's harder to change them later.
If syzbot already reported 100 bugs and we change titles, it will
start re-reporting the old one after new names and the old ones will
look stale, yet they still relevant, just detected under different
name.
So we also need to get this part right before enabling.


> I noticed a few things
> about the ubsan handlers: they don't use any of the common "warn"
> infrastructure (neither does kasan from what I can see), and was missing
> a check for panic_on_warn (kasan has this, but does it incorrectly).

Yes, panic_on_warn we also need.

I will look at the patches again for Acked-by.

> I think kasan and ubsan should be reworked to use the common warn
> infrastructure, and at the very least, ubsan needs this:
>
> diff --git a/lib/ubsan.c b/lib/ubsan.c
> index e7d31735950d..a2535a62c9af 100644
> --- a/lib/ubsan.c
> +++ b/lib/ubsan.c
> @@ -160,6 +160,17 @@ static void ubsan_epilogue(unsigned long *flags)
>                 "========================================\n");
>         spin_unlock_irqrestore(&report_lock, *flags);
>         current->in_ubsan--;
> +
> +       if (panic_on_warn) {
> +               /*
> +                * This thread may hit another WARN() in the panic path.
> +                * Resetting this prevents additional WARN() from panicking the
> +                * system on this thread.  Other threads are blocked by the
> +                * panic_mutex in panic().
> +                */
> +               panic_on_warn = 0;
> +               panic("panic_on_warn set ...\n");
> +       }
>  }
>
>  static void handle_overflow(struct overflow_data *data, void *lhs,
>
> > Kees, or anybody else interested, could you provide relevant configs
> > that (1) useful for kernel,
>
> As mentioned in the other email (but just to keep the note together with
> the other thoughts here) after this series, you'd want:
>
> CONFIG_UBSAN=y
> CONFIG_UBSAN_BOUNDS=y
> # CONFIG_UBSAN_MISC is not set
>
> > (2) we want 100% cleanliness,
>
> What do you mean here by "cleanliness"? It seems different from (3)
> about the test tripping a lot?
>
> > (3) don't
> > fire all the time even without fuzzing?
>
> I ran with the bounds checker enabled (and the above patch) under
> syzkaller for the weekend and saw 0 bounds checker reports.
>
> > Anything else required to
> > enable UBSAN? I don't see anything. syzbot uses gcc 8.something, which
> > I assume should be enough (but we can upgrade if necessary).
>
> As mentioned, gcc 8+ should be fine.
>
> --
> Kees Cook
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@xxxxxxxxxxxxxxxxx
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911262134.ED9E60965%40keescook.