On Mon, Jun 03, 2024 at 06:13:53PM UTC, Thomas Gleixner wrote:
> On Sat, Jun 01 2024 at 03:10, Gatlin Newhouse wrote:
>
> > Bring x86 to parity with arm64, similar to commit 25b84002afb9
> > ("arm64: Support Clang UBSAN trap codes for better reporting").
> > Enable the output of UBSAN type information on x86 architectures
> > compiled with clang when CONFIG_UBSAN_TRAP=y. Currently ARM
> > architectures output which specific sanitizer caused the trap,
> > via the 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.
>
> Please structure your change log properly instead of one paragraph of
> unstructured word salad. See:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> > +/*
> > + * Check for UD1, UD2, with or without Address Size Override Prefixes
> > instructions.
> > + */
> > __always_inline int is_valid_bugaddr(unsigned long addr)
> > {
> > if (addr < TASK_SIZE_MAX)
> > @@ -88,7 +92,13 @@ __always_inline int is_valid_bugaddr(unsigned long addr)
> > * We got #UD, if the text isn't readable we'd have gotten
> > * a different exception.
> > */
> > - return *(unsigned short *)addr == INSN_UD2;
> > + if (*(u16 *)addr == INSN_UD2)
> > + return INSN_UD2;
> > + if (*(u16 *)addr == INSN_UD1)
> > + return INSN_UD1;
> > + if (*(u8 *)addr == INSN_ASOP && *(u16 *)(addr + 1) == INSN_UD1)
>
> s/1/LEN_ASOP/ ?
>
> > + return INSN_ASOP;
> > + return 0;
>
> I'm not really a fan of the reuse of the INSN defines here. Especially
> not about INSN_ASOP. Also 0 is just lame.
>
> Neither does the function name make sense anymore. is_valid_bugaddr() is
> clearly telling that it's a boolean check (despite the return value
> being int for hysterical raisins). But now you turn it into a
> non-boolean integer which returns a instruction encoding. That's
> hideous. Programming should result in obvious code and that should be
> pretty obvious to people who create tools to validate code.
>
> Also all UBSAN cares about is the actual failure type and not the
> instruction itself:
>
> #define INSN_UD_MASK 0xFFFF
> #define INSN_ASOP_MASK 0x00FF
>
> #define BUG_UD_NONE 0xFFFF
> #define BUG_UD2 0xFFFE
>
> __always_inline u16 get_ud_type(unsigned long addr)
> {
> u16 insn;
>
> if (addr < TASK_SIZE_MAX)
> return BUD_UD_NONE;
>
> insn = *(u16 *)addr;
> if ((insn & INSN_UD_MASK) == INSN_UD2)
> return BUG_UD2;
>
> if ((insn & INSN_ASOP_MASK) == INSN_ASOP)
> insn = *(u16 *)(++addr);
>
> // UBSAN encodes the failure type in the two bytes after UD1
> if ((insn & INSN_UD_MASK) == INSN_UD1)
> return *(u16 *)(addr + LEN_UD1);
>
> return BUG_UD_NONE;
> }
>
> No?
Thanks for the feedback.
It seems that is_valid_bugaddr() needs to be implemented on all architectures
and the function get_ud_type() replaces it here. So how should the patch handle
is_valid_bugaddr()? Should the function remain as-is in traps.c despite no
longer being used?
>
> > static nokprobe_inline int
> > @@ -216,6 +226,7 @@ static inline void handle_invalid_op(struct pt_regs
> > *regs)
> > static noinstr bool handle_bug(struct pt_regs *regs)
> > {
> > bool handled = false;
> > + int insn;
> >
> > /*
> > * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
> > @@ -223,7 +234,8 @@ static noinstr bool handle_bug(struct pt_regs *regs)
> > * irqentry_enter().
> > */
> > kmsan_unpoison_entry_regs(regs);
> > - if (!is_valid_bugaddr(regs->ip))
> > + insn = is_valid_bugaddr(regs->ip);
> > + if (insn == 0)
>
> Sigh.
>
> But with the above sanitized (pun intended) this becomes obvious by
> itself:
>
> ud_type = get_ud_type(regs->ip);
> if (ud_type == BUG_UD_NONE)
> return false;
>
> See?
>
> > return handled;
> >
> > /*
> > @@ -236,10 +248,15 @@ static noinstr bool handle_bug(struct pt_regs *regs)
> > */
> > 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) {
>
> Please indent the second condition properly:
>
> if (a ||
> b) {
>
> I know you just added another tab, but when touching code, then please
> do it right.
>
> > + regs->ip += LEN_UD2;
> > + handled = true;
>
> > +/*
> > + * Checks for the information embedded in the UD1 trap instruction
> > + * for the UB Sanitizer in order to pass along debugging output.
> > + */
> > +void handle_ubsan_failure(struct pt_regs *regs, int insn)
> > +{
> > + u32 type = 0;
>
> Pointless initialization.
>
> > + if (insn == INSN_ASOP) {
> > + type = (*(u16 *)(regs->ip + LEN_ASOP + LEN_UD1));
> > + if ((type & 0xFF) == 0x40)
>
> No magic constants please. What does 0x40 mean?
>
> > + type = (type >> 8) & 0xFF;
>
> That mask is pointless as u16 is zero extended when assigned to u32, but
> why not using u16 in the first place to make it clear?
>
> > + } else {
> > + type = (*(u16 *)(regs->ip + LEN_UD1));
> > + if ((type & 0xFF) == 0x40)
> > + type = (type >> 8) & 0xFF;
> > + }
>
> Copy & pasta rules!
>
> unsigned long addr = regs->ip + LEN_UD1;
> u16 type;
>
> type = insn == INSN_UD1 ? *(u16 *)addr : *(u16 *)(addr + LEN_ASOP);
>
> if ((type & 0xFF) == UBSAN_MAGICALLY_USE_2ND_BYTE)
> type >>= 8;
> pr_crit("%s\n", report_ubsan_failure(regs, type));
>
> I don't see the point for printing regs->ip as this is followed by a
> stack trace anyway, but I don't have a strong opinion about it either.
>
> Though with the above get_ud_type() variant this becomes even simpler:
>
> void handle_ubsan_failure(struct pt_regs *regs, u16 type)
> {
> if ((type & 0xFF) == UBSAN_MAGICALLY_USE_2ND_BYTE)
> type >>= 8;
> pr_crit("%s\n", report_ubsan_failure(regs, type));
> }
>
> Thanks,
>
> tglx
