On Fri, Dec 12, 2025 at 02:47:57PM -0800, Andrew Pinski wrote: > On Tue, Dec 9, 2025 at 6:23 PM Kees Cook <[email protected]> wrote: > > Implement AArch64-specific KCFI backend. > [...] > > From an aarch64 point of view this is ok (except for a few minor > things [see below in the patch itself] which can be bundled up with > the fixes for the review of the middle-end parts and don't need an > extra approval).
Thanks! I will go through these and fix them up for v10. > The only open question with the aarch64 side of things (and other > targets) is how to represent the kfci calls, should there be wrapping > or not. > Right now I am ok with the wrapping but I am not a fan of it because > it introduces complexity to the call patterns. It is definitely > something that should be looked into but I am not going to block this > set of patches for it. > And using define_subst/define_subst_attr if we decide to stay with the > wrapping with the kfci rtl, is something to look into but that can > wait too. Yeah, I would love more guidance on this; Uros suggested using define_subst during the x86 review, and I was able to use it there because x86's expansion and call patterns don't use PARALLEL. When I tried to apply define_subst to the other architectures with PARALLEL it got extremely complex, and ended up being more complicated than what I already had. See this for my (brief) comments on this attempt in v7: https://lore.kernel.org/linux-hardening/[email protected]/ Maybe I just didn't see the right way to do it, so I'm happy to try something different here if there's a better method. > [...] > > +const char * > > +aarch64_output_kcfi_insn (rtx_insn *insn, rtx *operands) > > +{ > > + /* Target register is operands[0]. */ > > + rtx target_reg = operands[0]; > > + gcc_assert (REG_P (target_reg)); > > + > > + /* Get KCFI type ID from operand[3]. */ > > + uint32_t type_id = (uint32_t) INTVAL (operands[3]); > > Use UINTVAL . Now fixed (for all archs). > > + > > + /* Calculate typeid offset from call target. */ > > + HOST_WIDE_INT offset = -kcfi_get_typeid_offset (); > > + > > + /* Get unique label number for this KCFI check. */ > > + int labelno = kcfi_next_labelno (); > > + > > + /* Generate custom label names. */ > > + char trap_name[32]; > > + char call_name[32]; > > + ASM_GENERATE_INTERNAL_LABEL (trap_name, "Lkcfi_trap", labelno); > > + ASM_GENERATE_INTERNAL_LABEL (call_name, "Lkcfi_call", labelno); > > + > > + /* Load actual type into w16 from memory at offset using ldur. */ > > + rtx temp_operands[2]; > > + temp_operands[0] = target_reg; > > + temp_operands[1] = GEN_INT (offset); > > + output_asm_insn ("ldur\tw16, [%0, #%1]", temp_operands); > > + > > + /* Load expected type into w17 using mov/movk sequence. */ > > + fprintf (asm_out_file, "\tmov\tw17, #%u\n", type_id & 0xFFFF); > > + fprintf (asm_out_file, "\tmovk\tw17, #%u, lsl #16\n", (type_id >> 16) & > > 0xFFFF); > > + > > + /* Compare types. */ > > + fprintf (asm_out_file, "\tcmp\tw16, w17\n"); > > + > > + /* 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); > > + > > + /* Output trap label and BRK instruction. */ > > + ASM_OUTPUT_LABEL (asm_out_file, trap_name); > > + > > + /* Calculate and emit BRK with ESR encoding. */ > > + unsigned type_index = R17_REGNUM; > > + unsigned addr_index = REGNO (target_reg) - R0_REGNUM; > > + unsigned esr_value = 0x8000 | ((type_index & 31) << 5) | (addr_index & > > 31); > > + > > + fprintf (asm_out_file, "\tbrk\t#%u\n", esr_value); > > It might be useful to print the hex instead of the decimal here. > Either way is ok. Yeah, that's more human-readable. Now changed. -Kees -- Kees Cook
