Re: [PATCH 6/6] objtool: Validate kCFI calls

[Josh Poimboeuf]

On Mon, Apr 14, 2025 at 01:11:46PM +0200, Peter Zijlstra wrote:
>  SYM_FUNC_START(__efi_call)
> +	/*
> +	 * The EFI code doesn't have any CFI :-(
> +	 */
> +	ANNOTATE_NOCFI
>  	pushq %rbp
>  	movq %rsp, %rbp
>  	and $~0xf, %rsp

This looks like an insn annotation but is actually a func annotation.
ANNOTATE_NOCFI_SYM() would be a lot clearer, for all the asm code.

Though maybe it's better for ANNOTATE_NOCFI to annotate the call site
itself (for asm) while ANNOTATE_NOCFI_SYM annotates the entire function
(in C).  So either there would be two separate annotypes or the
annotation would get interpreted based on whether it's at the beginning
of the function.

> +++ b/include/linux/objtool.h
> @@ -185,6 +185,8 @@
>   */
>  #define ANNOTATE_REACHABLE(label)	__ASM_ANNOTATE(label, ANNOTYPE_REACHABLE)
>  
> +#define ANNOTATE_NOCFI_SYM(sym)		asm(__ASM_ANNOTATE(sym, ANNOTYPE_NOCFI))

This needs a comment like the others.

> @@ -2436,6 +2438,15 @@ static int __annotate_late(struct objtoo
>  		insn->_jump_table = (void *)1;
>  		break;
>  
> +	case ANNOTYPE_NOCFI:
> +		sym = insn->sym;
> +		if (!sym) {
> +			WARN_INSN(insn, "dodgy NOCFI annotation");
> +			break;

Return an error?

> @@ -4006,6 +4017,36 @@ static int validate_retpoline(struct obj
> +	/*
> +	 * kCFI call sites look like:
> +	 *
> +	 *     movl $(-0x12345678), %r10d
> +	 *     addl -4(%r11), %r10d
> +	 *     jz 1f
> +	 *     ud2
> +	 *  1: cs call __x86_indirect_thunk_r11
> +	 *
> +	 * Verify all indirect calls are kCFI adorned by checking for the
> +	 * UD2. Notably, doing __nocfi calls to regular (cfi) functions is
> +	 * broken.

This "__nocfi calls" is confusing me.  IIUC, there are two completely
different meanings for "nocfi":

  - __nocfi: disable the kcfi function entry stuff
  
  - ANNOTATE_NOCFI_SYM: Regardless of whether the function is __nocfi,
    allow it to have non-CFI indirect call sites.

Can we call this ANNOTATE_NOCFI_SAFE or something?

-- 
Josh