Re: [PATCH v3 4/7] aarch64: Add AArch64 Kernel Control Flow Integrity implementation

[Kees Cook <kees@kernel.org>]

On Sat, Sep 13, 2025 at 04:43:29PM -0700, Andrew Pinski wrote:
> On Sat, Sep 13, 2025 at 4:28 PM Kees Cook <kees@kernel.org> wrote:
> >
> > Implement AArch64-specific KCFI backend.
> >
> > - Trap debugging through ESR (Exception Syndrome Register) encoding
> >   in BRK instruction immediate values.
> >
> > - Scratch register allocation using w16/w17 (x16/x17) following
> >   AArch64 procedure call standard for intra-procedure-call registers.
> 
> How does this interact with BTI and sibcalls? Since for indirect
> calls, x17 is already used for the address.
> Why do you need/want to use a fixed register here for the load/compare
> anyways? Why can't you use any free register?

Ah, yeah, good point. I'm struggling with this on aarch32 too for Ard's
suggestion about using an eor sequence. So, the problem I haven't been
able to solve is that call instructions cannot have scratch register
operands. Or rather, can't when there is register pressure like on
aarch32, where a spill would be needed. This is in the LRA:

  if (CALL_P (curr_insn))
    no_output_reloads_p = true;

And that does make perfect sense, there's no place (in the current
design) to provide a place where the reload would happen. But I also
can't let the kcfi check/call get split up arbitrarily.

Is there some way I can convince LRA to let me do the restore manually?

> > +const char *
> > +aarch64_output_kcfi_insn (rtx_insn *insn, rtx *operands)
> > +{
> > +  /* KCFI is only supported in LP64 mode.  */
> > +  if (TARGET_ILP32)
> > +    {
> > +      sorry ("%<-fsanitize=kcfi%> is not supported for %<-mabi=ilp32%>");
> 
> You should reject -fsanitize=kcfi during option processing instead of
> this late in the compilation.

Where is best to do this on a per-arch basis?

> > +  /* Get KCFI type ID from operand[3].  */
> > +  uint32_t type_id = (uint32_t) INTVAL (operands[3]);
> 
> Maybe an assert that `(int32_t)type_id == INTVAL (operands[3])`?

Oh, hm, actually, I think I should be using UINTVAL instead?

> > +  /* Load actual type into w16 from memory at offset using ldur.  */
> > +  temp_operands[0] = gen_rtx_REG (SImode, R16_REGNUM);
> > +  temp_operands[1] = target_reg;
> > +  temp_operands[2] = GEN_INT (offset);
> > +  output_asm_insn ("ldur\t%w0, [%1, #%2]", temp_operands);
> 
> Since you are using a fixed register, you don't need the temp_operands[0] here.
> Also what happens if target_reg is x16? Shouldn't there be an assert
> on that here?

Yeah, I need to solve the scratch register issue more generally.

> > +  /* Output conditional branch to call label.  */
> > +  fputs ("\tb.eq\t", asm_out_file);
> > +  assemble_name (asm_out_file, call_name);
> > +  fputc ('\n', asm_out_file);
> 
> There has to be a better way of implementing this.

I couldn't find one that would let me keep the custom label name. I'd
love to have something better! :)


-- 
Kees Cook