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:

quoted

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).

quoted

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 <redacted>

[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.

quoted

+{
+	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.

quoted

[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.

quoted

+
+	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