| Message ID | 20191121181519.28637-1-keescook@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ubsan: Split out bounds checker | expand |
On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> 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@chromium.org > > 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! 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. Kees, or anybody else interested, could you provide relevant configs that (1) useful for kernel, (2) we want 100% cleanliness, (3) don't fire all the time even without fuzzing? 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). > Kees Cook (3): > ubsan: Add trap instrumentation option > ubsan: Split "bounds" checker from other options > lkdtm/bugs: Add arithmetic overflow and array bounds checks > > drivers/misc/lkdtm/bugs.c | 75 ++++++++++++++++++++++++++++++++++++++ > drivers/misc/lkdtm/core.c | 3 ++ > drivers/misc/lkdtm/lkdtm.h | 3 ++ > lib/Kconfig.ubsan | 42 +++++++++++++++++++-- > lib/Makefile | 2 + > scripts/Makefile.ubsan | 16 ++++++-- > 6 files changed, 134 insertions(+), 7 deletions(-) > > -- > 2.17.1
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@chromium.org> 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@chromium.org > > > > 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! > > 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. > Kees, or anybody else interested, could you provide relevant configs > that (1) useful for kernel, (2) we want 100% cleanliness, (3) don't > fire all the time even without fuzzing? 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). Nothing external should be needed; GCC and Clang support the ubsan options. Once this series lands, it should be possible to just enable this with: CONFIG_UBSAN=y CONFIG_UBSAN_BOUNDS=y # CONFIG_UBSAN_MISC is not set Based on initial testing, the bounds checker isn't very noisy, but I haven't spun up a syzbot instance to really confirm this yet (that was on the TODO list for today to let it run over the weekend).
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@chromium.org> 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@chromium.org > > > > 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? 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). 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.
On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> 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@chromium.org> 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@chromium.org > > > > > > 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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911262134.ED9E60965%40keescook.
On Wed, Nov 27, 2019 at 7:54 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> 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@chromium.org> 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@chromium.org > > > > > > > > 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. Acked-by: Dmitry Vyukov <dvyukov@google.com> for the series. I see you extended the test module, do you have samples of all UBSAN report types that are triggered by these functions? Is so, please add them to: https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report with whatever titles they are detected now. Improving titles will then be the next step, but much simpler with a good collection of tests. Will you send the panic_on_want patch as well? > > 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@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911262134.ED9E60965%40keescook.
On Wed, Nov 27, 2019 at 10:34:24AM +0100, Dmitry Vyukov wrote: > On Wed, Nov 27, 2019 at 7:54 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> 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@chromium.org> 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@chromium.org > > > > > > > > > > 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. It Sounds like instead of "UBSAN: Undefined behaviour in $file", UBSAN should report something like "UBSAN: $behavior in $file"? e.g. 40: UBSAN: bad shift in drivers/usb/core/devio.c:1517:25" 41: UBSAN: signed integer overflow in ./arch/x86/include/asm/atomic.h:156:2 I'll add one for the bounds checker. How are these reports used? (And is there a way to check a live kernel crash? i.e. to tell syzkaller "echo ARRAY_BOUNDS >/.../lkdtm..." and generate a report? > > > 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. > > > Acked-by: Dmitry Vyukov <dvyukov@google.com> > for the series. Thanks! > > I see you extended the test module, do you have samples of all UBSAN > report types that are triggered by these functions? Is so, please add > them to: > https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report Okay, cool. > with whatever titles they are detected now. Improving titles will then > be the next step, but much simpler with a good collection of tests. > > Will you send the panic_on_want patch as well? Yes; I wanted to make sure it was needed first (which you've confirmed now). I'll likely not send it until next week.
On Wed, Nov 27, 2019 at 6:59 PM Kees Cook <keescook@chromium.org> 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@chromium.org > > > > > > > > > > > > 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. > > It Sounds like instead of "UBSAN: Undefined behaviour in $file", UBSAN > should report something like "UBSAN: $behavior in $file"? > > e.g. > 40: UBSAN: bad shift in drivers/usb/core/devio.c:1517:25" > 41: UBSAN: signed integer overflow in ./arch/x86/include/asm/atomic.h:156:2 If you mean make them such that kernel testing systems could simply take the first line as "crash identity", then most likely we need function name there instead of file:line:column. At least this seems to be working the best based on our experience. > I'll add one for the bounds checker. > > How are these reports used? There are test inputs, each also contains expected parsing output (title at minimum, but can also contain crash type, corrupted mark, extracted "report") and that's verified against actual parsing result. > (And is there a way to check a live kernel > crash? i.e. to tell syzkaller "echo ARRAY_BOUNDS >/.../lkdtm..." and > generate a report? Unfortunately all of kernel tooling is completely untested at the moment. We would very much like to have all sanitizers tested in a meaningful way, e.g.: https://github.com/llvm-mirror/compiler-rt/blob/master/test/asan/TestCases/global-overflow.cpp#L15-L18 But also LOCKDEP, KMEMLEAK, ODEBUG, FAULT_INJECTS, etc, all untested too. Nobody knows what they produce, and if they even still detect bugs, report false positives, etc. But that's the kernel testing story... No, syzbot does not do kernels unit-testing. And there are no such tests anyways... > > > > 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. > > > > > > Acked-by: Dmitry Vyukov <dvyukov@google.com> > > for the series. > > Thanks! > > > > > I see you extended the test module, do you have samples of all UBSAN > > report types that are triggered by these functions? Is so, please add > > them to: > > https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report > > Okay, cool. > > > with whatever titles they are detected now. Improving titles will then > > be the next step, but much simpler with a good collection of tests. > > > > Will you send the panic_on_want patch as well? > > Yes; I wanted to make sure it was needed first (which you've confirmed > now). I'll likely not send it until next week. > > -- > 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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911270952.D66CD15AEC%40keescook.
On Wed, Nov 27, 2019 at 6:59 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Nov 27, 2019 at 10:34:24AM +0100, Dmitry Vyukov wrote: > > On Wed, Nov 27, 2019 at 7:54 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> 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@chromium.org> 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@chromium.org > > > > > > > > > > > > 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. > > It Sounds like instead of "UBSAN: Undefined behaviour in $file", UBSAN > should report something like "UBSAN: $behavior in $file"? > > e.g. > 40: UBSAN: bad shift in drivers/usb/core/devio.c:1517:25" > 41: UBSAN: signed integer overflow in ./arch/x86/include/asm/atomic.h:156:2 > > I'll add one for the bounds checker. > > How are these reports used? (And is there a way to check a live kernel > crash? i.e. to tell syzkaller "echo ARRAY_BOUNDS >/.../lkdtm..." and > generate a report? I've collected the sample and added to syzkaller test base: https://github.com/google/syzkaller/commit/76357d6f894431c00cc09cfc9e7474701a4b822a I also filed https://github.com/google/syzkaller/issues/1523 for enabling UBSAN on syzbot, let's move syzbot-related discussion there.
> On Nov 28, 2019, at 5:39 AM, 'Dmitry Vyukov' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > But also LOCKDEP, KMEMLEAK, ODEBUG, FAULT_INJECTS, etc, all untested > too. Nobody knows what they produce, and if they even still detect > bugs, report false positives, etc. > But that's the kernel testing story... Yes, those work except PROVE_LOCKING where there are existing potential deadlocks are almost impossible to fix them properly now. I have been running those for linux-next daily with all those debugging on where you can borrow the configs etc. https://github.com/cailca/linux-mm
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@chromium.org 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 Kees Cook (3): ubsan: Add trap instrumentation option ubsan: Split "bounds" checker from other options lkdtm/bugs: Add arithmetic overflow and array bounds checks drivers/misc/lkdtm/bugs.c | 75 ++++++++++++++++++++++++++++++++++++++ drivers/misc/lkdtm/core.c | 3 ++ drivers/misc/lkdtm/lkdtm.h | 3 ++ lib/Kconfig.ubsan | 42 +++++++++++++++++++-- lib/Makefile | 2 + scripts/Makefile.ubsan | 16 ++++++-- 6 files changed, 134 insertions(+), 7 deletions(-)