Re: [PATCH] x86/traps: Enable UBSAN traps on x86

[Kees Cook <keescook@chromium.org>]

On Wed, May 29, 2024 at 01:36:39PM -0700, Gatlin Newhouse wrote:
> On Wed, May 29, 2024 at 08:30:20PM UTC, Marco Elver wrote:
> > On Wed, 29 May 2024 at 20:17, Gatlin Newhouse <gatlin.newhouse@gmail.com> wrote:
> > >
> > > On Wed, May 29, 2024 at 09:25:21AM UTC, Marco Elver wrote:
> > > > On Wed, 29 May 2024 at 04:20, Gatlin Newhouse <gatlin.newhouse@gmail.com> wrote:
> > > > [...]
> > > > >         if (regs->flags & X86_EFLAGS_IF)
> > > > >                 raw_local_irq_enable();
> > > > > -       if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
> > > > > -           handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> > > > > -               regs->ip += LEN_UD2;
> > > > > -               handled = true;
> > > > > +
> > > > > +       if (insn == INSN_UD2) {
> > > > > +               if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
> > > > > +               handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> > > > > +                       regs->ip += LEN_UD2;
> > > > > +                       handled = true;
> > > > > +               }
> > > > > +       } else {
> > > > > +               if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> > > >
> > > > handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
> > > >
> > > > > +                       if (insn == INSN_REX)
> > > > > +                               regs->ip += LEN_REX;
> > > > > +                       regs->ip += LEN_UD1;
> > > > > +                       handled = true;
> > > > > +               }
> > > > >         }
> > > > >         if (regs->flags & X86_EFLAGS_IF)
> > > > >                 raw_local_irq_disable();
> > > > > diff --git a/arch/x86/kernel/ubsan.c b/arch/x86/kernel/ubsan.c
> > > > > new file mode 100644
> > > > > index 000000000000..6cae11f4fe23
> > > > > --- /dev/null
> > > > > +++ b/arch/x86/kernel/ubsan.c
> > > > > @@ -0,0 +1,32 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Clang Undefined Behavior Sanitizer trap mode support.
> > > > > + */
> > > > > +#include <linux/bug.h>
> > > > > +#include <linux/string.h>
> > > > > +#include <linux/printk.h>
> > > > > +#include <linux/ubsan.h>
> > > > > +#include <asm/ptrace.h>
> > > > > +#include <asm/ubsan.h>
> > > > > +
> > > > > +/*
> > > > > + * Checks for the information embedded in the UD1 trap instruction
> > > > > + * for the UB Sanitizer in order to pass along debugging output.
> > > > > + */
> > > > > +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
> > > > > +{
> > > > > +       u32 type = 0;
> > > > > +
> > > > > +       if (insn == INSN_REX) {
> > > > > +               type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
> > > > > +               if ((type & 0xFF) == 0x40)
> > > > > +                       type = (type >> 8) & 0xFF;
> > > > > +       } else {
> > > > > +               type = (*(u16 *)(regs->ip + LEN_UD1));
> > > > > +               if ((type & 0xFF) == 0x40)
> > > > > +                       type = (type >> 8) & 0xFF;
> > > > > +       }
> > > > > +       pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
> > > > > +
> > > > > +       return BUG_TRAP_TYPE_NONE;
> > > > > +}
> > > >
> > > > Shouldn't this return BUG_TRAP_TYPE_WARN?
> > >
> > > So as far as I understand, UBSAN trap mode never warns. Perhaps it does on
> > > arm64, although it calls die() so I am unsure. Maybe the condition in
> > > handle_bug() should be rewritten in the case of UBSAN ud1s? Do you have any
> > > suggestions?
> > 
> > AFAIK on arm64 it's basically a kernel OOPS.
> > 
> > The main thing I just wanted to point out though is that your newly added branch
> > 
> > > if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> > 
> > will never be taken, because I don't see where handle_ubsan_failure()
> > returns BUG_TRAP_TYPE_WARN.
> >
> 
> Initially I wrote this with some symmetry to the KCFI checks nearby, but I
> was unsure if this would be considered handled or not.

Yeah, that seemed like the right "style" to me too. Perhaps, since it
can never warn, we could just rewrite it so it's a void function avoid
the checking, etc.

-- 
Kees Cook