MIME-Version: 1.0
References: <20230203173946.gonna.972-kees@kernel.org> <CAFP8O3LdXO4-w57KeX+E2D2rOAv-bK47ur0=8XLgfEkXgB=5eg@mail.gmail.com>
 <63dd5f7a.170a0220.d72b.022f@mx.google.com>
In-Reply-To: <63dd5f7a.170a0220.d72b.022f@mx.google.com>
From:   Fangrui Song <maskray@google.com>
Date:   Fri, 3 Feb 2023 11:32:55 -0800
Message-ID: <CAFP8O3LX+fhgghnUiWD-U5UWOVsG9UPojJHXDPh0V2rdAURP1w@mail.gmail.com>
Subject: Re: [PATCH v2] arm64: Support Clang UBSAN trap codes for better reporting
To:     Kees Cook <keescook@chromium.org>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        John Stultz <jstultz@google.com>,
        Yongqin Liu <yongqin.liu@linaro.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        Yury Norov <yury.norov@gmail.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Marco Elver <elver@google.com>,
        linux-arm-kernel@lists.infradead.org, llvm@lists.linux.dev,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>,
        Alexander Potapenko <glider@google.com>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Feb 3, 2023 at 11:24 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Feb 03, 2023 at 11:14:49AM -0800, Fangrui Song wrote:
> > On Fri, Feb 3, 2023 at 9:39 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > When building with CONFIG_UBSAN_TRAP=3Dy on arm64, Clang encodes the =
UBSAN
> > > check (handler) type in the esr. Extract this and actually report the=
se
> > > traps as coming from the specific UBSAN check that tripped.
> > >
> > > Before:
> > >
> > >   Internal error: BRK handler: 00000000f20003e8 [#1] PREEMPT SMP
> > >
> > > After:
> > >
> > >   Internal error: UBSAN: shift out of bounds: 00000000f2005514 [#1] P=
REEMPT SMP
> > >
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: John Stultz <jstultz@google.com>
> > > Cc: Yongqin Liu <yongqin.liu@linaro.org>
> > > Cc: Sami Tolvanen <samitolvanen@google.com>
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Cc: Yury Norov <yury.norov@gmail.com>
> > > Cc: Andrey Konovalov <andreyknvl@gmail.com>
> > > Cc: Marco Elver <elver@google.com>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: llvm@lists.linux.dev
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > v2: improve commit log, limit report strings to actual configs, docum=
ent mappings
> > > v1: https://lore.kernel.org/lkml/20230202223653.never.473-kees@kernel=
.org/
> >
> > Thanks. I'll add the Linux kernel use to
> > https://maskray.me/blog/2023-01-29-all-about-undefined-behavior-sanitiz=
er
> > when this lands:)
>
> Oh nice post! Thanks for the pointer. :)
>
> >
> > > ---
> > >  arch/arm64/include/asm/brk-imm.h |  2 +
> > >  arch/arm64/kernel/traps.c        | 21 ++++++++++
> > >  include/linux/ubsan.h            |  9 +++++
> > >  lib/Makefile                     |  2 -
> > >  lib/ubsan.c                      | 67 ++++++++++++++++++++++++++++++=
++
> > >  lib/ubsan.h                      | 32 +++++++++++++++
> > >  6 files changed, 131 insertions(+), 2 deletions(-)
> > >  create mode 100644 include/linux/ubsan.h
> > >
> > > diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/as=
m/brk-imm.h
> > > index 6e000113e508..3f0f0d03268b 100644
> > > --- a/arch/arm64/include/asm/brk-imm.h
> > > +++ b/arch/arm64/include/asm/brk-imm.h
> > > @@ -28,6 +28,8 @@
> > >  #define BUG_BRK_IMM                    0x800
> > >  #define KASAN_BRK_IMM                  0x900
> > >  #define KASAN_BRK_MASK                 0x0ff
> > > +#define UBSAN_BRK_IMM                  0x5500
> > > +#define UBSAN_BRK_MASK                 0x00ff
> >
> > Q: How is 0x5500 derived?
>
> This is 'U' << 8 from:
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/AA=
rch64InstrInfo.td#L7571

Ah, yes. I forgot to check the code of 'U'

> > [...]
> > > +#ifdef CONFIG_UBSAN_TRAP
> > > +       register_kernel_break_hook(&ubsan_break_hook);
> > >  #endif
> >
> > IIUC, the break hook is a list so CONFIG_KASAN_SW_TAGS
> > (kernel-hwaddress) can be used with CONFIG_UBSAN_TRAP.
>
> Should I be doing something different here?
>
> > [...]
> > > +#ifdef CONFIG_UBSAN_ALIGNMENT
> > > +       /*
> > > +        * SanitizerKind::Alignment emits SanitizerHandler::TypeMisma=
tch
> > > +        * or SanitizerHandler::AlignmentAssumption.
> > > +        */
> > > +       case ubsan_alignment_assumption:
> > > +               return "UBSAN: alignment assumption";
> > > +       case ubsan_type_mismatch:
> > > +               return "UBSAN: type mismatch";
> > > +#endif
> > > +       default:
> > > +               return "UBSAN: unrecognized failure code";
> > > +       }
> > > +}
> >
> > I wonder whether keeping the dash-prefixed name is better since that
> > matches compiler-rt/lib/ubsan.
> > People can search for "add-overflow" and get cross references from
> > compiler-rt/lib/ubsan, instead of needing to knowing that "addition
> > overflow" is another name for "add-overflow".
>
> I think that the consumer of these messages wants as much plain-language
> detail as possible, so I'd prefer to expand these into full phrasing. To
> make it all more discoverable, I included all the details about how the
> mapping worked in the comments.

Ok, fine with me.

> > [...]
> > Reviewed-by: Fangrui Song <maskray@google.com>
>
> Thanks!
>
> --
> Kees Cook


--=20
=E5=AE=8B=E6=96=B9=E7=9D=BF