lkml.org 
[lkml]   [2021]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH 1/1] arm64: implement live patching
    From
    Date
    On Mon, 2021-06-07 at 11:20 +0100, Mark Rutland wrote:
    > On Fri, Jun 04, 2021 at 04:59:30PM -0700, Suraj Jitindar Singh wrote:
    > > It's my understanding that the two pieces of work required to
    > > enable live
    > > patching on arm are in flight upstream;
    > > - Reliable stack traces as implemented by Madhavan T. Venkataraman
    > > [1]
    > > - Objtool as implemented by Julien Thierry [2]
    >
    > We also need to rework the arm64 patching code to not rely on any
    > code
    > which may itself be patched. Currently there's a reasonable amount of
    > patching code that can either be patched directly, or can be
    > instrumented by code that can be patched.

    If I understand correctly you're saying that it's unsafe for patching
    code to call any other code (directly or indirectly) which can itself
    be patched.

    While I understand it's obviously important to fix this issue in the
    patching code as whole, live patching uses ftrace and as far as I can
    tell the only patching functions used by ftrace (in
    ftrace_modify_code()) is aarch64_insn_patch_text_nosync(). So would I
    be correct in my understanding that so long as that function doesn't
    call any other functions which can themselves be patched, then this
    would be safe?

    >
    > I have some WIP patches for that at:
    >
    >
    > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/patching/rework
    >
    > ... but there's still a lot to do, and it's not clear to me if
    > there's
    > other stuff we need to rework to make patch-safe.

    Is there any work I could contribute to this?

    >
    > Do we have any infrastructure for testing LIVEPATCH?

    Not currently. However I'm thinking something which attempted to
    trivially patch every patchable function would provide pretty good test
    coverage (while being very slow).

    >
    > > This is the remaining part required to enable live patching on arm.
    > > Based on work by Torsten Duwe [3]
    > >
    > > Allocate a task flag used to represent the patch pending state for
    > > the
    > > task. Also implement generic functions klp_arch_set_pc() &
    > > klp_get_ftrace_location().
    > >
    > > In klp_arch_set_pc() it is sufficient to set regs->pc as in
    > > ftrace_common_return() the return address is loaded from the stack.
    > >
    > > ldr x9, [sp, #S_PC]
    > > <snip>
    > > ret x9
    > >
    > > In klp_get_ftrace_location() it is necessary to advance the address
    > > by
    > > AARCH64_INSN_SIZE (4) to point to the BL in the callsite as 2 nops
    > > were
    > > placed at the start of the function, one to be patched to save the
    > > LR and
    > > another to be patched to branch to the ftrace call, and
    > > klp_get_ftrace_location() is expected to return the address of the
    > > BL. It
    > > may also be necessary to advance the address by another
    > > AARCH64_INSN_SIZE
    > > if CONFIG_ARM64_BTI_KERNEL is enabled due to the instruction placed
    > > at the
    > > branch target to satisfy BTI,
    > >
    > > Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
    > >
    > > [1] https://lkml.org/lkml/2021/5/26/1212
    > > [2] https://lkml.org/lkml/2021/3/3/1135
    > > [3] https://lkml.org/lkml/2018/10/26/536
    > > ---
    > > arch/arm64/Kconfig | 3 ++
    > > arch/arm64/include/asm/livepatch.h | 42
    > > ++++++++++++++++++++++++++++
    > > arch/arm64/include/asm/thread_info.h | 4 ++-
    > > arch/arm64/kernel/signal.c | 4 +++
    > > 4 files changed, 52 insertions(+), 1 deletion(-)
    > > create mode 100644 arch/arm64/include/asm/livepatch.h
    > >
    > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
    > > index b098dabed8c2..c4636990c01d 100644
    > > --- a/arch/arm64/Kconfig
    > > +++ b/arch/arm64/Kconfig
    > > @@ -187,6 +187,7 @@ config ARM64
    > > select HAVE_GCC_PLUGINS
    > > select HAVE_HW_BREAKPOINT if PERF_EVENTS
    > > select HAVE_IRQ_TIME_ACCOUNTING
    > > + select HAVE_LIVEPATCH
    > > select HAVE_NMI
    > > select HAVE_PATA_PLATFORM
    > > select HAVE_PERF_EVENTS
    > > @@ -1946,3 +1947,5 @@ source "arch/arm64/kvm/Kconfig"
    > > if CRYPTO
    > > source "arch/arm64/crypto/Kconfig"
    > > endif
    > > +
    > > +source "kernel/livepatch/Kconfig"
    > > diff --git a/arch/arm64/include/asm/livepatch.h
    > > b/arch/arm64/include/asm/livepatch.h
    > > new file mode 100644
    > > index 000000000000..72d7cd86f158
    > > --- /dev/null
    > > +++ b/arch/arm64/include/asm/livepatch.h
    > > @@ -0,0 +1,42 @@
    > > +/* SPDX-License-Identifier: GPL-2.0
    > > + *
    > > + * livepatch.h - arm64-specific Kernel Live Patching Core
    > > + */
    > > +#ifndef _ASM_ARM64_LIVEPATCH_H
    > > +#define _ASM_ARM64_LIVEPATCH_H
    > > +
    > > +#include <linux/ftrace.h>
    > > +
    > > +static inline void klp_arch_set_pc(struct ftrace_regs *fregs,
    > > unsigned long ip)
    > > +{
    > > + struct pt_regs *regs = ftrace_get_regs(fregs);
    > > +
    > > + regs->pc = ip;
    > > +}
    > > +
    > > +/*
    > > + * klp_get_ftrace_location is expected to return the address of
    > > the BL to the
    > > + * relevant ftrace handler in the callsite. The location of this
    > > can vary based
    > > + * on several compilation options.
    > > + * CONFIG_DYNAMIC_FTRACE_WITH_REGS
    > > + * - Inserts 2 nops on function entry the second of which is the
    > > BL
    > > + * referenced above. (See ftrace_init_nop() for the callsite
    > > sequence)
    > > + * (this is required by livepatch and must be selected)
    > > + * CONFIG_ARM64_BTI_KERNEL:
    > > + * - Inserts a hint #0x22 on function entry if the function is
    > > called
    > > + * indirectly (to satisfy BTI requirements), which is inserted
    > > before
    > > + * the two nops from above.
    > > + */
    > > +#define klp_get_ftrace_location klp_get_ftrace_location
    > > +static inline unsigned long klp_get_ftrace_location(unsigned long
    > > faddr)
    >
    > Is `faddr` the address of the function, or the addresds of the patch
    > site (the
    > dyn_ftrace::ip)? Either way, I think there's a problem; see below.
    >

    It is the address of the function.

    > > +{
    > > + unsigned long addr = faddr + AARCH64_INSN_SIZE;
    > > +
    > > +#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)
    > > + addr = ftrace_location_range(addr, addr + AARCH64_INSN_SIZE);
    > > +#endif
    >
    > I don't think this is right; function are not guaranteed to have a
    > BTI,
    > since e.g. static functions which are called directly may not be
    > given
    > one:

    Correct, gcc only inserts the instruction on functions which it
    determines are called indirectly and thus validated.

    >
    > > [mark@lakrids:/mnt/data/tests/bti-patching]% cat test.c
    > > #define noinline __attribute__((noinline))
    > >
    > > static noinline void a(void)
    > > {
    > > asm volatile("" ::: "memory");
    > > }
    > >
    > > void b(void)
    > > {
    > > a();
    > > }
    > > [mark@lakrids:/mnt/data/tests/bti-patching]% usekorg 10.3.0
    > > aarch64-linux-gcc -c test.c -fpatchable-function-entry=2 -mbranch-
    > > protection=bti -O2
    > > [mark@lakrids:/mnt/data/tests/bti-patching]% usekorg 10.3.0
    > > aarch64-linux-objdump -d
    > > test.o
    > >
    > > test.o: file format elf64-littleaarch64
    > >
    > >
    > > Disassembly of section .text:
    > >
    > > 0000000000000000 <a>:
    > > 0: d503201f nop
    > > 4: d503201f nop
    > > 8: d65f03c0 ret
    > > c: d503201f nop
    > >
    > > 0000000000000010 <b>:
    > > 10: d503245f bti c
    > > 14: d503201f nop
    > > 18: d503201f nop
    > > 1c: 17fffff9 b 0 <a>
    >
    > If `faddr` is the address of the function, then we'll need to do
    > something dynamic to skip any BTI. If it's the address of the patch
    > site, then we shouldn't need to consider the BTI at all: att boot
    > time
    > the recorded lcoation points at the first NOP, and at init time we
    > point
    > dyn_ftrace::ip at the second NOP -- see ftrace_call_adjust().

    faddr is the address of the function.
    This concern is what my code attempts to address.

    Either way we need the address of the branch which is AARCH64_INSN_SIZE
    after the function address if BTI is _disabled_.
    If BTI is _enabled_ the branch could be either at the address we just
    computed or AARCH64_INSN_SIZE after it depending on if the instruction
    was inserted by the compiler. This is why ftrace_location_range() is
    used as it finds the correct ftrace branch location within the
    specified range.
    So the range is from
    faddr + AARCH64_INSN_SIZE (BTI disabled or enabled & not inserted)
    to
    faddr + 2 * AARCH64_INSN_SIZE (BTI enabled and instr inserted)

    >
    > Thanks,
    > Mark.

    Thanks for taking a look.
    Suraj.

    >
    > > +
    > > + return addr;
    > > +}
    > > +
    > > +#endif /* _ASM_ARM64_LIVEPATCH_H */
    > > diff --git a/arch/arm64/include/asm/thread_info.h
    > > b/arch/arm64/include/asm/thread_info.h
    > > index 6623c99f0984..cca936d53a40 100644
    > > --- a/arch/arm64/include/asm/thread_info.h
    > > +++ b/arch/arm64/include/asm/thread_info.h
    > > @@ -67,6 +67,7 @@ int arch_dup_task_struct(struct task_struct *dst,
    > > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep
    > > */
    > > #define TIF_MTE_ASYNC_FAULT 5 /* MTE Asynchronous Tag
    > > Check Fault */
    > > #define TIF_NOTIFY_SIGNAL 6 /* signal notifications exist */
    > > +#define TIF_PATCH_PENDING 7 /* pending live patching update */
    > > #define TIF_SYSCALL_TRACE 8 /* syscall trace active */
    > > #define TIF_SYSCALL_AUDIT 9 /* syscall auditing */
    > > #define TIF_SYSCALL_TRACEPOINT 10 /* syscall tracepoint for
    > > ftrace */
    > > @@ -97,11 +98,12 @@ int arch_dup_task_struct(struct task_struct
    > > *dst,
    > > #define _TIF_SVE (1 << TIF_SVE)
    > > #define _TIF_MTE_ASYNC_FAULT (1 << TIF_MTE_ASYNC_FAULT)
    > > #define _TIF_NOTIFY_SIGNAL (1 << TIF_NOTIFY_SIGNAL)
    > > +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING)
    > >
    > > #define _TIF_WORK_MASK (_TIF_NEED_RESCHED |
    > > _TIF_SIGPENDING | \
    > > _TIF_NOTIFY_RESUME |
    > > _TIF_FOREIGN_FPSTATE | \
    > > _TIF_UPROBE | _TIF_MTE_ASYNC_FAULT | \
    > > - _TIF_NOTIFY_SIGNAL)
    > > + _TIF_NOTIFY_SIGNAL |
    > > _TIF_PATCH_PENDING)
    > >
    > > #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE |
    > > _TIF_SYSCALL_AUDIT | \
    > > _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP
    > > | \
    > > diff --git a/arch/arm64/kernel/signal.c
    > > b/arch/arm64/kernel/signal.c
    > > index 6237486ff6bb..d1eedb0589a7 100644
    > > --- a/arch/arm64/kernel/signal.c
    > > +++ b/arch/arm64/kernel/signal.c
    > > @@ -18,6 +18,7 @@
    > > #include <linux/sizes.h>
    > > #include <linux/string.h>
    > > #include <linux/tracehook.h>
    > > +#include <linux/livepatch.h>
    > > #include <linux/ratelimit.h>
    > > #include <linux/syscalls.h>
    > >
    > > @@ -932,6 +933,9 @@ asmlinkage void do_notify_resume(struct pt_regs
    > > *regs,
    > > (void __user *)NULL,
    > > current);
    > > }
    > >
    > > + if (thread_flags & _TIF_PATCH_PENDING)
    > > + klp_update_patch_state(current);
    > > +
    > > if (thread_flags & (_TIF_SIGPENDING |
    > > _TIF_NOTIFY_SIGNAL))
    > > do_signal(regs);
    > >
    > > --
    > > 2.17.1
    > >

    \
     
     \ /
      Last update: 2021-06-09 02:33    [W:3.063 / U:0.196 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site