Re: [PATCH v3 06/10] x86/traps: Decode LOCK Jcc.d8 #UD

[Kees Cook]

On Wed, Feb 19, 2025 at 05:21:13PM +0100, Peter Zijlstra wrote:
> Because overlapping code sequences are all the rage.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>

Reviewed-by: Kees Cook <kees@xxxxxxxxxx>

Semi-pointless stream of consciousness below...

> ---
>  arch/x86/include/asm/bug.h |    2 ++
>  arch/x86/kernel/traps.c    |   30 +++++++++++++++++++++++++-----
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -17,6 +17,7 @@
>   * In clang we have UD1s reporting UBSAN failures on X86, 64 and 32bit.
>   */
>  #define INSN_ASOP		0x67
> +#define INSN_LOCK		0xf0
>  #define OPCODE_ESCAPE		0x0f
>  #define SECOND_BYTE_OPCODE_UD1	0xb9
>  #define SECOND_BYTE_OPCODE_UD2	0x0b
> @@ -26,6 +27,7 @@
>  #define BUG_UD1			0xfffd
>  #define BUG_UD1_UBSAN		0xfffc
>  #define BUG_EA			0xffea
> +#define BUG_LOCK		0xfff0
>  
>  #ifdef CONFIG_GENERIC_BUG
>  
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -97,6 +97,7 @@ __always_inline int is_valid_bugaddr(uns
>   * If it's a UD1, further decode to determine its use:
>   *
>   * FineIBT:      ea                      (bad)
> + * FineIBT:      0f 75 f9                lock jne . - 6
>   * UBSan{0}:     67 0f b9 00             ud1    (%eax),%eax
>   * UBSan{10}:    67 0f b9 40 10          ud1    0x10(%eax),%eax
>   * static_call:  0f b9 cc                ud1    %esp,%ecx
> @@ -106,6 +107,7 @@ __always_inline int is_valid_bugaddr(uns
>  __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len)
>  {
>  	unsigned long start = addr;
> +	bool lock = false;
>  	u8 v;
>  
>  	if (addr < TASK_SIZE_MAX)
> @@ -114,12 +116,29 @@ __always_inline int decode_bug(unsigned
>  	v = *(u8 *)(addr++);
>  	if (v == INSN_ASOP)
>  		v = *(u8 *)(addr++);
> -	if (v == 0xea) {
> +
> +	if (v == INSN_LOCK) {
> +		lock = true;
> +		v = *(u8 *)(addr++);
> +	}
> +
> +	switch (v) {
> +	case 0x70 ... 0x7f: /* Jcc.d8 */
> +		addr += 1; /* d8 */
> +		*len = addr - start;
> +		WARN_ON_ONCE(!lock);
> +		return BUG_LOCK;
> +
> +	case 0xea:
>  		*len = addr - start;
>  		return BUG_EA;
> -	}
> -	if (v != OPCODE_ESCAPE)
> +
> +	case OPCODE_ESCAPE:
> +		break;
> +
> +	default:
>  		return BUG_NONE;
> +	}
>  
>  	v = *(u8 *)(addr++);
>  	if (v == SECOND_BYTE_OPCODE_UD2) {
> @@ -315,7 +334,8 @@ static noinstr bool handle_bug(struct pt
>  
>  	switch (ud_type) {
>  	case BUG_EA:
> -		if (handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> +	case BUG_LOCK:
> +		if (handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
>  			if (regs->ip == addr)
>  				regs->ip += ud_len;
>  			handled = true;
> @@ -324,7 +344,7 @@ static noinstr bool handle_bug(struct pt
>  
>  	case BUG_UD2:
>  		if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
> -		    handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> +		    handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
>  			if (regs->ip == addr)
>  				regs->ip += ud_len;
>  			handled = true;

I realize these are misplaced chunks, but passing ud_type into the
handler feels like a layering violation to me. I struggled with this
when making recommendations for the UBSAN handler too, so I'm not sure
I have any better idea. It feels like there should be a way to separate
this logic more cleanly. The handlers are all doing very similar things:

1- find the address where a bad thing happened
2- report about it
3- whether to continue execution
4- where to continue execution

The variability happens with 1 and 4, where it depends on the instruction
sequences. Meh, I dunno. I can't see anything cleaner, so passing down
ud_type does seem best.

-- 
Kees Cook