linux-kernel - Re: [PATCH v3] x86/traps: Enable UBSAN traps on x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 20:04:41 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Kees Cook <kees@...nel.org>
Cc: Gatlin Newhouse <gatlin.newhouse@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	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>, 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>,
	"Mike Rapoport (IBM)" <rppt@...nel.org>,
	Baoquan He <bhe@...hat.com>,
	Rick Edgecombe <rick.p.edgecombe@...el.com>,
	Changbin Du <changbin.du@...wei.com>,
	Pengfei Xu <pengfei.xu@...el.com>, Xin Li <xin3.li@...el.com>,
	Jason Gunthorpe <jgg@...pe.ca>, Uros Bizjak <ubizjak@...il.com>,
	Arnd Bergmann <arnd@...db.de>,
	"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 v3] x86/traps: Enable UBSAN traps on x86

On Wed, Jun 26, 2024 at 12:07:52PM -0700, Kees Cook wrote:
> On Tue, Jun 25, 2024 at 11:37:19AM +0200, Peter Zijlstra wrote:
> > Also, wouldn't it be saner to write this something like:
> > 
> > __always_inline int decode_bug(unsigned long addr, u32 *imm)
> > {
> > 	u8 v;
> > 
> > 	if (addr < TASK_SIZE)
> > 		return BUG_NONE;
> > 
> > 	v = *(u8 *)(addr++);
> > 	if (v == 0x67)
> > 		v = *(u8 *)(addr++);
> > 	if (v != 0x0f)
> > 		return BUG_NONE;
> > 	v = *(u8 *)(addr++);
> > 	if (v == 0x0b)
> > 		return BUG_UD2;
> > 	if (v != 0xb9)
> > 		return BUG_NONE;
> > 

Looks like I lost:

	v = *(u8 *)(addr++);
> > 	if (X86_MODRM_RM(v) == 4)
> > 		addr++; /* consume SiB */
> > 
> > 	*imm = 0;
> > 	if (X86_MODRM_MOD(v) == 1)
> > 		*imm = *(u8 *)addr;
> > 	if (X86_MORRM_MOD(v) == 2)
> > 		*imm = *(u32 *)addr;
> > 
> > 	// WARN on MOD(v)==3 ??
> > 
> > 	return BUG_UD1;
> > }
> 
> Thanks for the example! (I think it should use macros instead of
> open-coded "0x67", "0x0f", etc, but yeah.)

Yeah, I didn't feel like hunting down pre-existing defines for all of
them, but yeah.

> > Why does the thing emit the asop prefix at all through? afaict it
> > doesn't affect the immediate you want to get at. And if it does this
> > prefix, should we worry about other prefixes? Ideally we'd not accept
> > any prefixes.
> 
> AFAICT it's because it's a small immediate? For an x86_64 build, this is
> how Clang is generating the UD1.

So the disp8 immediate comes from MOD==1, MOD==2 has a disp32. What the
prefix does is change the size of the memory being referenced from 32bit
to 16bit iirc, but since UD does not actually perform the load, this is
entirely superfluous afaict.

It might be good to figure out *why* clang thinks it needs this.

A REX prefix is far more likely to be useful (upper 8 destination
register for instance).

Anyway, it seems to basically boil down to needing a fairly complete
instruction decoder without being able the use the normal one :/