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

[Gatlin Newhouse]

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@xxxxxxxxx> 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@xxxxxxxxx> 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.

> 
> That means 'handled' will be false, and the code in exc_invalid_op
> will proceed to call handle_invalid_op() which is probably not what
> you intended - i.e. it's definitely not BUG_TRAP_TYPE_NONE, but one of
> TYPE_WARN of TYPE_BUG.
>

This remains a question to me as to whether it should be considered handled
or not. Which is why I'm happy to change this branch which is never taken to
something else that still outputs the UBSAN type information before calling
handle_invalid_op().

> 
> Did you test it and you got the behaviour you expected?
>

Testing with LKDTM provided the output I expected. The UBSAN type information
along with file and offsets are output before an illegal op and trace.