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

[Kees Cook <kees@kernel.org>]

On Thu, Jul 11, 2024 at 11:06:12AM +0200, Marco Elver wrote:
> On Thu, 11 Jul 2024 at 10:10, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Jul 10, 2024 at 08:32:38PM +0000, Gatlin Newhouse wrote:
> > > Currently ARM architectures extract which specific sanitizer
> > > has caused a trap via encoded data in the trap instruction.
> > > Clang on x86 currently encodes the same data in ud1 instructions
> > > but the x86 handle_bug() and is_valid_bugaddr() functions
> > > currently only look at ud2s.
> > >
> > > Bring x86 to parity with arm64, similar to commit 25b84002afb9
> > > ("arm64: Support Clang UBSAN trap codes for better reporting").
> > > Enable the reporting of UBSAN sanitizer detail on x86 architectures
> > > compiled with clang when CONFIG_UBSAN_TRAP=y.
> >
> > Can we please get some actual words on what code clang will generate for
> > this? This doesn't even refer to the clang commit.
> >
> > How am I supposed to know if the below patch matches what clang will
> > generate etc..
> 
> I got curious what the history of this is - I think it was introduced
> in https://github.com/llvm/llvm-project/commit/c5978f42ec8e9, which
> was reviewed here: https://reviews.llvm.org/D89959

Sorry, I should have suggested this commit be mentioned in the commit
log. The details are in llvm/lib/Target/X86/X86MCInstLower.cpp:
https://github.com/llvm/llvm-project/commit/c5978f42ec8e9#diff-bb68d7cd885f41cfc35843998b0f9f534adb60b415f647109e597ce448e92d9f

  case X86::UBSAN_UD1:
    EmitAndCountInstruction(MCInstBuilder(X86::UD1Lm)
                                .addReg(X86::EAX)
                                .addReg(X86::EAX)
                                .addImm(1)
                                .addReg(X86::NoRegister)
                                .addImm(MI->getOperand(0).getImm())
                                .addReg(X86::NoRegister));

Which is using the UD1Lm template from
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86InstrSystem.td#L27

  def UD1Lm   : I<0xB9, MRMSrcMem, (outs), (ins GR32:$src1, i32mem:$src2),
                  "ud1{l}\t{$src2, $src1|$src1, $src2}", []>, TB, OpSize32;

It uses OpSize32, distinct from UD1Wm (16) and UD1Qm (64).

> But besides that, there's very little documentation. Either Gatlin or
> one of the other LLVM folks might have more background, but we might
> be out of luck if that 1 commit is all there is.
> 
> [+Cc Tim, author of the LLVM commit]
> 
> > > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> > > index a3ec87d198ac..ccd573d58edb 100644
> > > --- a/arch/x86/include/asm/bug.h
> > > +++ b/arch/x86/include/asm/bug.h
> > > @@ -13,6 +13,17 @@
> > >  #define INSN_UD2     0x0b0f
> > >  #define LEN_UD2              2
> > >
> > > +/*
> > > + * In clang we have UD1s reporting UBSAN failures on X86, 64 and 32bit.
> > > + */
> > > +#define INSN_ASOP    0x67
> >
> > I asked, but did not receive answer, *WHY* does clang add this silly
> > prefix? AFAICT this is entirely spurious and things would be simpler if
> > we don't have to deal with it.

Even if we change LLVM, I'd still like to support the older versions, so
we'll need to handle this regardless.

> >
> > > +#define OPCODE_PREFIX        0x0f
> >
> > This is *NOT* a prefix, it is an escape, please see the SDM Vol 2
> > Chapter 'Instruction Format'. That ASOP thing above is a prefix.
> >
> > > +#define OPCODE_UD1   0xb9
> > > +#define OPCODE_UD2   0x0b
> >
> > These are second byte opcodes. The actual (single byte opcodes) of those
> > value exist and are something entirely different (0xB0+r is MOV, and
> > 0x0B is OR).

What would be your preferred names for all of these defines?

-- 
Kees Cook