Kernel-hardening archive on lore.kernel.org
 help / color / Atom feed
From: Dmitry Vyukov <dvyukov@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	 Elena Petrova <lenaptr@google.com>,
	Alexander Potapenko <glider@google.com>,
	 Linus Torvalds <torvalds@linux-foundation.org>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	 "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	Arnd Bergmann <arnd@arndb.de>,
	 Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	kasan-dev <kasan-dev@googlegroups.com>,
	 LKML <linux-kernel@vger.kernel.org>,
	kernel-hardening@lists.openwall.com,
	 syzkaller <syzkaller@googlegroups.com>
Subject: Re: [PATCH v2 0/3] ubsan: Split out bounds checker
Date: Wed, 27 Nov 2019 07:54:25 +0100
Message-ID: <CACT4Y+bsLJ-wFx_TaXqax3JByUOWB3uk787LsyMVcfW6JzzGvg@mail.gmail.com> (raw)
In-Reply-To: <201911262134.ED9E60965@keescook>

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.

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-21 18:15 Kees Cook
2019-11-21 18:15 ` [PATCH v2 1/3] ubsan: Add trap instrumentation option Kees Cook
2019-12-16 10:26   ` Will Deacon
2019-12-18  0:08     ` Kees Cook
2019-11-21 18:15 ` [PATCH v2 2/3] ubsan: Split "bounds" checker from other options Kees Cook
2019-11-21 18:15 ` [PATCH v2 3/3] lkdtm/bugs: Add arithmetic overflow and array bounds checks Kees Cook
2019-11-22  9:07 ` [PATCH v2 0/3] ubsan: Split out bounds checker Dmitry Vyukov
2019-11-22 16:52   ` Kees Cook
2019-11-27  5:42   ` Kees Cook
2019-11-27  6:54     ` Dmitry Vyukov [this message]
2019-11-27  9:34       ` Dmitry Vyukov
2019-11-27 17:59         ` Kees Cook
2019-11-28 10:38           ` Dmitry Vyukov
2019-11-28 16:14             ` Qian Cai
2019-11-28 13:10           ` Dmitry Vyukov

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACT4Y+bsLJ-wFx_TaXqax3JByUOWB3uk787LsyMVcfW6JzzGvg@mail.gmail.com \
    --to=dvyukov@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=ard.biesheuvel@linaro.org \
    --cc=arnd@arndb.de \
    --cc=aryabinin@virtuozzo.com \
    --cc=dan.carpenter@oracle.com \
    --cc=glider@google.com \
    --cc=gustavo@embeddedor.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=lenaptr@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=syzkaller@googlegroups.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel-hardening archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernel-hardening/0 kernel-hardening/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernel-hardening kernel-hardening/ https://lore.kernel.org/kernel-hardening \
		kernel-hardening@lists.openwall.com
	public-inbox-index kernel-hardening

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.openwall.lists.kernel-hardening


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git