lkml.org 
[lkml]   [2021]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6 2/2] arm64: implement support for static call trampolines
    Hi Ard,

    On Fri, Nov 05, 2021 at 03:59:17PM +0100, Ard Biesheuvel wrote:
    > +static void *strip_cfi_jt(void *addr)
    > +{
    > + if (IS_ENABLED(CONFIG_CFI_CLANG)) {
    > + void *p = addr;
    > + u32 insn;
    > +
    > + /*
    > + * Taking the address of a function produces the address of the
    > + * jump table entry when Clang CFI is enabled. Such entries are
    > + * ordinary jump instructions, preceded by a BTI C instruction
    > + * if BTI is enabled for the kernel.
    > + */
    > + if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
    > + p += 4;
    > +
    > + insn = le32_to_cpup(p);
    > + if (aarch64_insn_is_b(insn))
    > + return p + aarch64_get_branch_offset(insn);
    > +
    > + WARN_ON(1);
    > + }
    > + return addr;
    > +}

    I'm somewhat uncomfortable with this, because it seems like the compiler could
    easily violate our expectations in future, and then we're in for a massive
    headache. I assume clang doesn't provide any guarnatee as to the format of the
    jump table entries (and e.g. I can see scope for branch padding breaking this).

    In trying to sidestep that I ended up with:

    https://lore.kernel.org/linux-arm-kernel/20211109172408.49641-1-mark.rutland@arm.com/

    ... which I think is a good option for PREEMPT_DYNAMIC, but I don't know if
    there were other places where we believe static calls would be critical for
    performance rather than a nice-to-have, and whether we truly need static calls
    on arm64. My mind is leaning towards "avoid if reasonable" at the moment (or at
    least make that mutually exclusive with CFI so we can avoid that specific fun).

    I see you had at least one other user in:

    https://lore.kernel.org/r/20211109120336.3561463-1-ardb@kernel.org

    ... what were your thoughts on the criticality of that?

    FWIW other than the above this looks good to me. My major concern here is
    fragility/maintenance, and secondly whether we're gaining much in practice. So
    if you think we really need this, I'm not going to stand in the way.

    Thanks
    Mark.

    > +void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
    > +{
    > + /*
    > + * -0x8 <literal>
    > + * 0x0 bti c <--- trampoline entry point
    > + * 0x4 <branch or nop>
    > + * 0x8 ldr x16, <literal>
    > + * 0xc cbz x16, 20
    > + * 0x10 br x16
    > + * 0x14 ret
    > + */
    > + struct {
    > + u64 literal;
    > + __le32 insn[2];
    > + } insns;
    > + u32 insn;
    > + int ret;
    > +
    > + insn = aarch64_insn_gen_hint(AARCH64_INSN_HINT_BTIC);
    > + insns.literal = (u64)func;
    > + insns.insn[0] = cpu_to_le32(insn);
    > +
    > + if (!func) {
    > + insn = aarch64_insn_gen_branch_reg(AARCH64_INSN_REG_LR,
    > + AARCH64_INSN_BRANCH_RETURN);
    > + } else {
    > + insn = aarch64_insn_gen_branch_imm((u64)tramp + 4,
    > + (u64)strip_cfi_jt(func),
    > + AARCH64_INSN_BRANCH_NOLINK);
    > +
    > + /*
    > + * Use a NOP if the branch target is out of range, and rely on
    > + * the indirect call instead.
    > + */
    > + if (insn == AARCH64_BREAK_FAULT)
    > + insn = aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP);
    > + }
    > + insns.insn[1] = cpu_to_le32(insn);
    > +
    > + ret = __aarch64_insn_write(tramp - 8, &insns, sizeof(insns));
    > + if (!WARN_ON(ret))
    > + caches_clean_inval_pou((u64)tramp - 8, sizeof(insns));
    > }
    >
    > int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
    > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
    > index 50bab186c49b..e16860a14eaf 100644
    > --- a/arch/arm64/kernel/vmlinux.lds.S
    > +++ b/arch/arm64/kernel/vmlinux.lds.S
    > @@ -173,6 +173,7 @@ SECTIONS
    > HIBERNATE_TEXT
    > KEXEC_TEXT
    > TRAMP_TEXT
    > + STATIC_CALL_TEXT
    > *(.gnu.warning)
    > . = ALIGN(16);
    > *(.got) /* Global offset table */
    > --
    > 2.30.2
    >

    \
     
     \ /
      Last update: 2021-11-09 18:56    [W:2.609 / U:0.724 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site