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

[Peter Zijlstra <peterz@infradead.org>]

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 :/