Date: Tue, 11 Jun 2024 13:26:09 -0700
From: Gatlin Newhouse <gatlin.newhouse@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, 
	Kees Cook <keescook@...omium.org>, Marco Elver <elver@...gle.com>, 
	Andrey Konovalov <andreyknvl@...il.com>, Andrey Ryabinin <ryabinin.a.a@...il.com>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Rick Edgecombe <rick.p.edgecombe@...el.com>, 
	Baoquan He <bhe@...hat.com>, Changbin Du <changbin.du@...wei.com>, 
	Pengfei Xu <pengfei.xu@...el.com>, Josh Poimboeuf <jpoimboe@...nel.org>, Xin Li <xin3.li@...el.com>, 
	Jason Gunthorpe <jgg@...pe.ca>, Tina Zhang <tina.zhang@...el.com>, 
	Uros Bizjak <ubizjak@...il.com>, "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>, 
	linux-kernel@...r.kernel.org, kasan-dev@...glegroups.com, linux-hardening@...r.kernel.org, 
	llvm@...ts.linux.dev
Subject: Re: [PATCH v2] x86/traps: Enable UBSAN traps on x86

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

Powered by blists - more mailing lists