Re: [PATCH 00/11] x86/ibt: FineIBT-BHI

[Sami Tolvanen <samitolvanen@google.com>]

On Thu, Feb 13, 2025 at 12:45:47PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 13, 2025 at 11:48:02AM +0100, Peter Zijlstra wrote:
> 
> > > One thing I realized is that CONFIG_CFI_PERMISSIVE doesn't actually do
> > > anything when FineIBT is used since we lose track of CFI trap
> > > locations. I'm not sure if that's worth fixing, but perhaps we could
> > > disable FineIBT when permissive mode is enabled to avoid confusion?
> > 
> > Hmm, yeah, that's one thing I keep forgetting about. Let me try and fix
> > it and see how ugly it gets before offering an opinion :-)
> 
> Completely untested and on top of this series minus the last two patches
> -- basically what I just pushed into:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/core
> 
> WDYT?
> 
> ---
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index e285933506e9..1fec1e445a25 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1410,6 +1410,36 @@ static void poison_cfi(void *addr)
>  	}
>  }
>  
> +bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
> +{
> +	/*
> +	 * FineIBT preamble:
> +	 *
> +	 *  __cfi_foo:
> +	 *     endbr64
> +	 *     subl $0x12345678, %r10d
> +	 *     jz   1f
> +	 *     ud2
> +	 *  1: nop
> +	 *  foo:
> +	 *
> +	 * regs->ip points to the UD2 instruction.
> +	 */
> +	unsigned long addr = regs->ip - (4+7+2);
> +	u32 hash;
> +
> +	if (!is_endbr((void *)addr)) {
> +Efault:
> +		return false;
> +	}
> +
> +	*target = addr + fineibt_preamble_size;
> +
> +	__get_kernel_nofault(&hash, *(u32 *)(addr + fineibt_preamble_hash), u32, Efault);

You have an extra * here, should be just (u32 *).

> +	*type = (u32)regs->r10 + hash;
> +	return true;
> +}
> +
>  #else
>  
>  static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
> @@ -1421,6 +1451,11 @@ static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
>  static void poison_cfi(void *addr) { }
>  #endif
>  
> +bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
> +{
> +	return false;
> +}
> +
>  #endif
>  
>  void apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
> diff --git a/arch/x86/kernel/cfi.c b/arch/x86/kernel/cfi.c
> index e6bf78fac146..4c682076c656 100644
> --- a/arch/x86/kernel/cfi.c
> +++ b/arch/x86/kernel/cfi.c
> @@ -61,6 +61,8 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
>  	return true;
>  }
>  
> +extern bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type);
> +
>  /*
>   * Checks if a ud2 trap is because of a CFI failure, and handles the trap
>   * if needed. Returns a bug_trap_type value similarly to report_bug.
> @@ -70,11 +72,25 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
>  	unsigned long target;
>  	u32 type;
>  
> -	if (!is_cfi_trap(regs->ip))
> -		return BUG_TRAP_TYPE_NONE;
> +	switch (cfi_mode) {
> +	case CFI_KCFI:
> +		if (!is_cfi_trap(regs->ip))
> +			return BUG_TRAP_TYPE_NONE;
> +
> +		if (!decode_cfi_insn(regs, &target, &type))
> +			return report_cfi_failure_noaddr(regs, regs->ip);
> +
> +		break;
>  
> -	if (!decode_cfi_insn(regs, &target, &type))
> -		return report_cfi_failure_noaddr(regs, regs->ip);
> +	case CFI_FINEIBT:
> +		if (!decode_fineibt_insn(regs, &target, &type))
> +			return BUG_TRAP_TYPE_NONE;
> +
> +		break;
> +
> +	default:
> +		return BUG_TRAP_TYPE_NONE;
> +	}
>  
>  	return report_cfi_failure(regs, regs->ip, &target, type);
>  }

Otherwise, LGTM.

One minor issue is that since the trap is in the preamble, we don't
get caller information in the warning message:

[   19.080184] CFI failure at __cfi_lkdtm_increment_int+0xd/0x10
(target: lkdtm_increment_int+0x0/0x20; expected type: 0x7e0c52a5)

But this is followed by a call trace, so it's not really necessary
either. With the __get_kernel_nofault argument fixed:

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami