arm: Add livepatch support

klp_get_ftrace_location is used by ftrace to get the entry for a specific function from the mcount list. klp_arch_set_pc is used to set the pc from the regs passed as an argument to the ftrace_ops_no_ops function to the starting address of the patched function. klp_write_module_reloc is not doing anything at this moment.

Signed-off-by Abel Vesa <[email protected]>

---
 MAINTAINERS                      |  3 +++
 arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 arch/arm/include/asm/livepatch.h
 create mode 100644 arch/arm/kernel/livepatch.c

MAINTAINERS +1 -0

index bd182a1..d43b790 100644
@@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <[email protected]>
 M:	Jessica Yu <[email protected]>
 M:	Jiri Kosina <[email protected]>
 M:	Miroslav Benes <[email protected]>
+M:	Abel Vesa <[email protected]>
 R:	Petr Mladek <[email protected]>

arch/arm/include/asm/livepatch.h new file +23 -0

new file mode 100644
index 0000000..d4e3ff0
@@ -0,0 +1,46 @@
+/*
+ * livepatch.h - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_ARM_LIVEPATCH_H
+#define _ASM_ARM_LIVEPATCH_H
+
+#include <asm/setup.h>

arch/arm/include/asm/livepatch.h new file +3 -0

+}
+
+#endif /* _ASM_ARM_LIVEPATCH_H */

arch/arm/kernel/livepatch.c new file +33 -0

new file mode 100644
index 0000000..0656cd6
@@ -0,0 +1,43 @@
+/*
+ * livepatch.c - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/ftrace.h>
+#include <asm/elf.h>
+#include <asm/livepatch.h>
+#include <asm/insn.h>
+#include <asm/ftrace.h>
+
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod:	module in which the section to be modified is found
+ * @type:	ELF relocation type (see asm/elf.h)
+ * @loc:	address that the relocation should be written to
+ * @value:	relocation value (sym address + addend)

Quoted 27 lines

klp_get_ftrace_location is used by ftrace to get the entry for a specific function from the mcount list. klp_arch_set_pc is used to set the pc from the regs passed as an argument to the ftrace_ops_no_ops function to the starting address of the patched function. klp_write_module_reloc is not doing anything at this moment.

Signed-off-by Abel Vesa <[email protected]>

---
 MAINTAINERS                      |  3 +++
 arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 arch/arm/include/asm/livepatch.h
 create mode 100644 arch/arm/kernel/livepatch.c

MAINTAINERS +1 -0

index bd182a1..d43b790 100644
@@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <[email protected]>
 M:	Jessica Yu <[email protected]>
 M:	Jiri Kosina <[email protected]>
 M:	Miroslav Benes <[email protected]>
+M:	Abel Vesa <[email protected]>
 R:	Petr Mladek <[email protected]>

MAINTAINERS M: Josh Poimboeuf <[email protected]> · line 7480

Quoted 10 lines

MAINTAINERS +2 -0

 S:	Maintained
 F:	kernel/livepatch/
 F:	include/linux/livepatch.h
 F:	arch/x86/include/asm/livepatch.h
 F:	arch/x86/kernel/livepatch.c
+F:	arch/arm/include/asm/livepatch.h
+F:	arch/arm/kernel/livepatch.c
 F:	Documentation/livepatch/
 F:	Documentation/ABI/testing/sysfs-kernel-livepatch
 F:	samples/livepatch/

Thanks for the offer but I think we manage quite well ;)

Quoted 29 lines

arch/arm/include/asm/livepatch.h new file +23 -0

new file mode 100644
index 0000000..d4e3ff0
@@ -0,0 +1,46 @@
+/*
+ * livepatch.h - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_ARM_LIVEPATCH_H
+#define _ASM_ARM_LIVEPATCH_H
+
+#include <asm/setup.h>

arch/arm/include/asm/livepatch.h line 33

Quoted 10 lines

arch/arm/include/asm/livepatch.h new file +10 -0

+#include <linux/module.h>
+#include <linux/ftrace.h>
+
+static inline int klp_check_compiler_support(void)
+{
+	return 0;
+}
+
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+			   unsigned long loc, unsigned long value);

This is not needed. See below.

arch/arm/include/asm/livepatch.h line 43

Quoted 10 lines

arch/arm/include/asm/livepatch.h new file +10 -0

+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+	regs->uregs[15] = ip;
+}
+
+#define klp_get_ftrace_location klp_get_ftrace_location
+static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
+{
+	return ftrace_location_range(faddr, faddr + 24);

Comment here about +24 would be great. See similar in powerpc header file.

Quoted 50 lines

arch/arm/include/asm/livepatch.h new file +3 -0

+}
+
+#endif /* _ASM_ARM_LIVEPATCH_H */

arch/arm/kernel/livepatch.c new file +41 -0

new file mode 100644
index 0000000..0656cd6
@@ -0,0 +1,43 @@
+/*
+ * livepatch.c - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/ftrace.h>
+#include <asm/elf.h>
+#include <asm/livepatch.h>
+#include <asm/insn.h>
+#include <asm/ftrace.h>
+
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod:	module in which the section to be modified is found
+ * @type:	ELF relocation type (see asm/elf.h)
+ * @loc:	address that the relocation should be written to
+ * @value:	relocation value (sym address + addend)
+ *
+ * This function writes a relocation to the specified location for
+ * a particular module.
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+			   unsigned long loc, unsigned long value)
+{
+	/* Not implemented yet */

arch/arm/kernel/livepatch.c line 43

Quoted 2 lines

arch/arm/kernel/livepatch.c new file +2 -0

+	return 0;
+}

This whole file is not needed. Livepatching uses kernel's internal apply_relocate_add() for dealing with relocations. The only thing you need to do is to make sure that all needed arch-specific info is preserved during a module loading. Specifically mod_arch_specific structure needs to be examined in this respect.

But apply_relocate_add() is not implemented on arm yet. I guess it would nice to have it... otherwise we could get to an unpleasant situation. Livepatch module can rely on its livepatching relocations (that is, there are some). apply_relocate_add() returns 0 on arm, so everything seems to be nice and then boom some time later.

The question is what happens with normal modules. There are no SHT_RELA records probably.

+++ Miroslav Benes [16/01/17 17:47 +0100]:

Quoted 146 lines · Abel Vesa

Quoted 34 lines

klp_get_ftrace_location is used by ftrace to get the entry for a specific function from the mcount list. klp_arch_set_pc is used to set the pc from the regs passed as an argument to the ftrace_ops_no_ops function to the starting address of the patched function. klp_write_module_reloc is not doing anything at this moment.

Signed-off-by Abel Vesa <[email protected]>

---

MAINTAINERS                      |  3 +++
arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
3 files changed, 92 insertions(+)

create mode 100644 arch/arm/include/asm/livepatch.h create mode 100644 arch/arm/kernel/livepatch.c

MAINTAINERS +0 -0

index bd182a1..d43b790 100644
@@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <[email protected]>

M: Jessica Yu <[email protected]>
M: Jiri Kosina <[email protected]>
M: Miroslav Benes <[email protected]>
+M: Abel Vesa <[email protected]>
R: Petr Mladek <[email protected]>
S: Maintained
F: kernel/livepatch/
F: include/linux/livepatch.h
F: arch/x86/include/asm/livepatch.h
F: arch/x86/kernel/livepatch.c

MAINTAINERS +2 -0

+F:	arch/arm/include/asm/livepatch.h
+F:	arch/arm/kernel/livepatch.c

Quoted 3 lines

F: Documentation/livepatch/
F: Documentation/ABI/testing/sysfs-kernel-livepatch
F: samples/livepatch/

Thanks for the offer but I think we manage quite well ;)

Quoted 29 lines

arch/arm/include/asm/livepatch.h new file +23 -0

new file mode 100644
index 0000000..d4e3ff0
@@ -0,0 +1,46 @@
+/*
+ * livepatch.h - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_ARM_LIVEPATCH_H
+#define _ASM_ARM_LIVEPATCH_H
+
+#include <asm/setup.h>

arch/arm/include/asm/livepatch.h line 33

Quoted 10 lines

arch/arm/include/asm/livepatch.h new file +10 -0

+#include <linux/module.h>
+#include <linux/ftrace.h>
+
+static inline int klp_check_compiler_support(void)
+{
+	return 0;
+}
+
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+			   unsigned long loc, unsigned long value);

This is not needed. See below.

arch/arm/include/asm/livepatch.h line 43

Quoted 10 lines

arch/arm/include/asm/livepatch.h new file +10 -0

+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+	regs->uregs[15] = ip;
+}
+
+#define klp_get_ftrace_location klp_get_ftrace_location
+static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
+{
+	return ftrace_location_range(faddr, faddr + 24);

Comment here about +24 would be great. See similar in powerpc header file.

Quoted 50 lines

arch/arm/include/asm/livepatch.h new file +3 -0

+}
+
+#endif /* _ASM_ARM_LIVEPATCH_H */

arch/arm/kernel/livepatch.c new file +41 -0

new file mode 100644
index 0000000..0656cd6
@@ -0,0 +1,43 @@
+/*
+ * livepatch.c - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/ftrace.h>
+#include <asm/elf.h>
+#include <asm/livepatch.h>
+#include <asm/insn.h>
+#include <asm/ftrace.h>
+
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod:	module in which the section to be modified is found
+ * @type:	ELF relocation type (see asm/elf.h)
+ * @loc:	address that the relocation should be written to
+ * @value:	relocation value (sym address + addend)
+ *
+ * This function writes a relocation to the specified location for
+ * a particular module.
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+			   unsigned long loc, unsigned long value)
+{
+	/* Not implemented yet */

Quoted 8 lines

arch/arm/kernel/livepatch.c line 43

Quoted 2 lines

arch/arm/kernel/livepatch.c new file +2 -0

+	return 0;
+}

This whole file is not needed. Livepatching uses kernel's internal apply_relocate_add() for dealing with relocations. The only thing you need to do is to make sure that all needed arch-specific info is preserved during a module loading. Specifically mod_arch_specific structure needs to be examined in this respect.

Yup. Since we rely on apply_relocate_add() to apply relocations, just make sure that you preserve anything (e.g., plt tables) you need to make that call post-module init. In the case of s390, we needed to make sure we kept mod->arch.syminfo, so that the call to apply_relocate_add() would succeed.

Quoted 5 lines

But apply_relocate_add() is not implemented on arm yet. I guess it would nice to have it... otherwise we could get to an unpleasant situation. Livepatch module can rely on its livepatching relocations (that is, there are some). apply_relocate_add() returns 0 on arm, so everything seems to be nice and then boom some time later.

Doesn't it return -ENOEXEC? MODULES_USE_ELF_RELA is not defined on arm, I think (see moduleloader.h).

Quoted 6 lines

But apply_relocate_add() is not implemented on arm yet. I guess it would nice to have it... otherwise we could get to an unpleasant situation. Livepatch module can rely on its livepatching relocations (that is, there are some). apply_relocate_add() returns 0 on arm, so everything seems to be nice and then boom some time later.

Doesn't it return -ENOEXEC? MODULES_USE_ELF_RELA is not defined on arm, I think (see moduleloader.h).

Quoted 2 lines

The question is what happens with normal modules. There are no SHT_RELA records probably.

For arm, I think there are only SHT_REL relocation sections (MODULES_USE_ELF_REL is set instead of MODULES_USE_ELF_RELA), so during apply_relocations() in load_module(), only apply_relocate() should be called, and not apply_relocate_add().

Hm, I guess that means if we want livepatch support for the architectures that don't support RELA relocations, we would have to check if the section is SHT_REL or SHT_RELA before calling the right apply_relocate* function.

It was only added to fix compiler error. It is not implemented yet.

Signed-off-by Abel Vesa <[email protected]>

---
arch/arm/kernel/module.c | 9 +++++++++

1 file changed, 9 insertions(+)

arch/arm/kernel/module.c +0 -0

index 4f14b5c..bf94922 100644
@@ -52,6 +52,15 @@ void *module_alloc(unsigned long size)
 #endif
 

int

+++ Abel Vesa [06/12/16 17:06 +0000]:

Quoted 16 lines

It was only added to fix compiler error. It is not implemented yet.

Signed-off-by Abel Vesa <[email protected]>

---
arch/arm/kernel/module.c | 9 +++++++++

1 file changed, 9 insertions(+)

arch/arm/kernel/module.c +0 -0

index 4f14b5c..bf94922 100644
@@ -52,6 +52,15 @@ void *module_alloc(unsigned long size)
 #endif
 

int

arch/arm/kernel/module.c void *module_alloc(unsigned long size) · line 61

Quoted 7 lines

arch/arm/kernel/module.c +7 -0

+apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
+		   unsigned int symindex, unsigned int relindex,
+		   struct module *module)
+{
+	/* Not implemented yet */
+	return 0;
+}

Are SHT_RELA relocation sections actually supported on 32-bit arm? It looks like there's only support for SHT_REL.

arch/arm/Kconfig:84: select MODULES_USE_ELF_REL

If we support SHT_REL sections, the correct fix is to probably have klp_write_object_relocations() check if the relocation section is SHT_REL or SHT_RELA, and call the appropriate function (apply_relocate or apply_relocate_add), similar to how the module loader does it.

This adds __ftrace_regs_caller which, unlike __ftrace_caller, adds register saving/restoring and livepatch handling if the pc register gets modified by klp_ftrace_handler.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/entry-ftrace.S | 49

++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)

(quoted diff) +0 -0

b/arch/arm/kernel/entry-ftrace.S index c73c403..b6ada5c 100644

arch/arm/kernel/entry-ftrace.S +10 -0

@@ -92,6 +92,46 @@
 2:	mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller suffix
+
+	stmdb	sp!, {r0-r15}
+	mov	r3, sp
+
+	ldr	r10, [sp, #60]
+
+	mcount_get_lr   r1                      @ lr of instrumented

func

arch/arm/kernel/entry-ftrace.S +1 -0

+	mcount_adjust_addr	r0, lr		@

instrumented function +

arch/arm/kernel/entry-ftrace.S +18 -0

+#endif
+ftrace_regs_caller_end\suffix:
+	ldmia	sp!, {r0-r14}
+	add	sp, sp, #4
+	mov	pc, lr
+.endm
+
+#endif
+
 .macro __ftrace_caller suffix
 	mcount_enter

@@ -212,6 +252,15 @@ UNWIND(.fnstart)
 	__ftrace_caller
 UNWIND(.fnend)
 ENDPROC(ftrace_caller)
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ENTRY(ftrace_regs_caller)
+UNWIND(.fnstart)
+	__ftrace_regs_caller
+UNWIND(.fnend)
+ENDPROC(ftrace_regs_caller)
+#endif
+
 #endif
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER

This adds __ftrace_regs_caller which, unlike __ftrace_caller, adds register saving/restoring and livepatch handling if the pc register gets modified by klp_ftrace_handler.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

arch/arm/kernel/entry-ftrace.S +21 -0

index c73c403..b6ada5c 100644
@@ -92,6 +92,46 @@
 2:	mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller suffix
+
+	stmdb	sp!, {r0-r15}
+	mov	r3, sp
+
+	ldr	r10, [sp, #60]
+
+	mcount_get_lr   r1                      @ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call\suffix
+ftrace_regs_call\suffix:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_regs_graph_call\suffix
+ftrace_regs_graph_call\suffix:
+	mov	r0, r0
+#endif

This adds __ftrace_regs_caller which, unlike __ftrace_caller, adds register saving/restoring and livepatch handling if the pc register gets modified by klp_ftrace_handler.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

arch/arm/kernel/entry-ftrace.S +0 -0

index c73c403..b6ada5c 100644

arch/arm/kernel/entry-ftrace.S +19 -0

+	mov	r3, sp
+
+	ldr	r10, [sp, #60]
+
+	mcount_get_lr   r1                      @ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call\suffix
+ftrace_regs_call\suffix:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_regs_graph_call\suffix
+ftrace_regs_graph_call\suffix:
+	mov	r0, r0
+#endif
+#ifdef CONFIG_LIVEPATCH
+	ldr	r0, [sp, #60]
+	cmp	r0, r10

arch/arm/kernel/entry-ftrace.S +15 -0

+	add	sp, sp, #4
+	mov	pc, lr
+.endm
+
+#endif
+
 .macro __ftrace_caller suffix
 	mcount_enter
 

@@ -212,6 +252,15 @@ UNWIND(.fnstart)
 	__ftrace_caller
 UNWIND(.fnend)
 ENDPROC(ftrace_caller)
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ENTRY(ftrace_regs_caller)
+UNWIND(.fnstart)
+	__ftrace_regs_caller
+UNWIND(.fnend)
+ENDPROC(ftrace_regs_caller)
+#endif
+
 #endif
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER

Hi Abel,

Quoted 14 lines · Abel Vesa

This adds __ftrace_regs_caller which, unlike __ftrace_caller, adds register saving/restoring and livepatch handling if the pc register gets modified by klp_ftrace_handler.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

arch/arm/kernel/entry-ftrace.S +0 -0

index c73c403..b6ada5c 100644

arch/arm/kernel/entry-ftrace.S line 99

Quoted 8 lines

arch/arm/kernel/entry-ftrace.S +5 -0

@@ -92,6 +92,46 @@
 2:	mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller suffix
+
+	stmdb	sp!, {r0-r15}

As the kbuild robot reported, you can't do this. For ARM, it's unknown what the value stored for r13 will be, and for a Thumb-2 kernel the whole instruction is flat out unpredictable (i.e. it might just undef or anything).

Quoted 19 lines

arch/arm/kernel/entry-ftrace.S +19 -0

+	mov	r3, sp
+
+	ldr	r10, [sp, #60]
+
+	mcount_get_lr   r1                      @ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call\suffix
+ftrace_regs_call\suffix:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_regs_graph_call\suffix
+ftrace_regs_graph_call\suffix:
+	mov	r0, r0
+#endif
+#ifdef CONFIG_LIVEPATCH
+	ldr	r0, [sp, #60]
+	cmp	r0, r10

arch/arm/kernel/entry-ftrace.S line 128

Quoted 10 lines

arch/arm/kernel/entry-ftrace.S +10 -0

+	beq	ftrace_regs_caller_end
+	ldmia	sp!, {r0-r12}
+	add	sp, sp, #8
+	ldmia	sp!, {r11}
+	sub	sp, r12, #16
+	str	r11, [sp, #12]
+	ldmia	sp!, {r11, r12, lr, pc}
+#endif
+ftrace_regs_caller_end\suffix:
+	ldmia	sp!, {r0-r14}

Again, the value of SP at this point is now unknown (regardless of whether what the value on memory was correct or not). Not good.

Either use non-writeback addressing modes, or avoid saving/restoring SP at all (AFAICS it isn't necessary, since if the SP was changed in between, you then wouldn't know where to restore the saved registers from anyway).

Robin.

Quoted 26 lines

arch/arm/kernel/entry-ftrace.S +15 -0

+	add	sp, sp, #4
+	mov	pc, lr
+.endm
+
+#endif
+
 .macro __ftrace_caller suffix
 	mcount_enter
 

@@ -212,6 +252,15 @@ UNWIND(.fnstart)
 	__ftrace_caller
 UNWIND(.fnend)
 ENDPROC(ftrace_caller)
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ENTRY(ftrace_regs_caller)
+UNWIND(.fnstart)
+	__ftrace_regs_caller
+UNWIND(.fnend)
+ENDPROC(ftrace_regs_caller)
+#endif
+
 #endif
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER

Necessary livepatch file added to makefile.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/Makefile | 1 +
 1 file changed, 1 insertion(+)

arch/arm/kernel/Makefile +0 -0

index ad325a8..9e70220 100644

Quoted 12 lines · Abel Vesa

Necessary livepatch file added to makefile.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/Makefile | 1 +
 1 file changed, 1 insertion(+)

arch/arm/kernel/Makefile +0 -0

index ad325a8..9e70220 100644

arch/arm/kernel/Makefile obj-$(CONFIG_HAVE_ARM_TWD) += smp_twd.o · line 51

Quoted 4 lines

arch/arm/kernel/Makefile +1 -0

@@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
 obj-$(CONFIG_ARM_ARCH_TIMER)	+= arch_timer.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= entry-ftrace.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
+obj-$(CONFIG_LIVEPATCH)		+= livepatch.o

It is strange that you add a source file in one patch and make it build in a much later patch.

I suggest to restructure the entire patchset a bit. Please, first add support for FTRACE_WITH_REGS. It makes sense on its own. Then add the livepatch support on top of it.

Otherwise, it is not necessary to send v2 immediately for such non-trivial code. There might be more people that would want to look at it and it might take days until they find a time. It is always better to collect some feedback, think about it over night(s). Every question often opens many other questions and it usually takes some time until all settles down into a good picture again.

Best Regards,
Petr

Necessary livepatch file added to makefile.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/kernel/Makefile | 1 +
 1 file changed, 1 insertion(+)

arch/arm/kernel/Makefile +0 -0

index ad325a8..9e70220 100644

This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH to arm Kconfig.

Signed-off-by Abel Vesa <[email protected]>

This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH to arm Kconfig.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm/Kconfig +2 -0

index 186c4c2..f4e9ace 100644
@@ -50,6 +50,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)

@@ -67,6 +68,7 @@ config ARM
 	select HAVE_KERNEL_XZ
 	select HAVE_KPROBES if !XIP_KERNEL && !CPU_ENDIAN_BE32 && !CPU_V7M
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
+	select HAVE_LIVEPATCH
 	select HAVE_MEMBLOCK
 	select HAVE_MOD_ARCH_SPECIFIC
 	select HAVE_NMI

arch/arm/Kconfig config ARM · line 87

Quoted 5 lines · Abel Vesa

arch/arm/Kconfig +1 -0

@@ -82,6 +84,7 @@ config ARM
 	select HAVE_VIRT_CPU_ACCOUNTING_GEN
 	select IRQ_FORCED_THREADING
 	select MODULES_USE_ELF_REL
+	select MODULES_USE_ELF_RELA

I wrote it before but I think you cannot have both MODULES_USE_ELF_REL and MODULES_USE_ELF_RELA.

This is just an idea I've been trying out for a while now.

Just in case somebody wants to play with it, this applies to linux-arm/for-next.

Also please note that this was only tested in qemu, but I will do some testing on some real hardware in the following days.

FWICT, on this arch the compiler always generates a function prologue somewhere between these lines:

e1a0c00d        mov     ip, sp
e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
e24cb004        sub     fp, ip, #4
e24dd064        sub     sp, sp, #100    ; 0x64<--- local variables
e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
ebf9c2c9        bl      80110364<__gnu_mcount_nc>

....

Every function that follows this pattern (the number of registers pushed and the sp subtraction for the local variables being the only acceptable exception) can be patched with this mechanism. IIRC, only the inline functions and notrace functions do not follow this pattern.

Considering that the function is livepatchable, when the time comes to call ftrace_call, the ftrace_regs_caller is called instead.

Because this arch didn't have a ftrace with regs implementation, the ftrace_regs_caller was added.

This new function adds the regs saving/restoring part, plus the part necessary for the livepatch mechanism to work. After the regs are saved and the r3 is set to contain the sp's value, we're keeping the old pc into r10 in order to be checked later against the new pc.

Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called and the klp_ftrace_handler overwrites the old pc with the new one.

Here comes the tricky part. We're checking if the pc is still the old one, if it is we jump the whole livepatching and go ahead with restoring the saved regs.

If the pc is modified, it means we're livepatching current function and we need to pop all regs from r1 through r12, jump over the next two regs saved on stack (we're not interested in those since we're trying to get the same regs context as it was at the point the function-to-be-patched was called) and put the new pc into r11.

Since r12 contains the sp from when the function just got branched to, we need to set the sp back to that.

Then we need to put the new pc on stack so that when we're popping r11 through pc, we will actually jump to the first instruction from the new function.

We don't need to worry about the returning phase since the epilogue of the new function will take care of that and from there on everything goes back to normal.

The whole advantage of this over adding compiler support is that we're not introducing nops at the beginning of the function. As a matter of fact, we're not changing anything between an image with livepatch and an image without it (except the ftrace_regs_call addition and the livepatch necessary code).

As for the implementation of the ftrace_regs_caller, I still think there might be some unsafe stack handling since I'm getting some build warnings. Those are due to pushing/popping of a list of regs in which the sp resides. I'll try to get around those in a next iteration (if necessary), but first I would like to hear some opinions about this work and if it's worth going forward.

Everything else should be pretty straightforward, so I'll skip explaining that.

Abel Vesa (7):
arm: Add livepatch arch specific code
arm: ftrace: Add call modify mechanism
arm: module: Add apply_relocate_add
arm: Add ftrace with regs support
arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
arm: Add livepatch to build if CONFIG_LIVEPATCH
arm: Add livepatch necessary arch selects into Kconfig

  MAINTAINERS                      |  3 +++
  arch/arm/Kconfig                 |  4 ++++
  arch/arm/include/asm/ftrace.h    |  4 ++++
  arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
  arch/arm/kernel/Makefile         |  1 +
  arch/arm/kernel/entry-ftrace.S   | 49 ++++++++++++++++++++++++++++++++++++++++
  arch/arm/kernel/ftrace.c         | 21 +++++++++++++++++
  arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++
  arch/arm/kernel/module.c         |  9 ++++++++
  9 files changed, 180 insertions(+)

create mode 100644 arch/arm/include/asm/livepatch.h create mode 100644 arch/arm/kernel/livepatch.c

This is just an idea I've been trying out for a while now.

Just in case somebody wants to play with it, this applies to linux-arm/for-next.

Also please note that this was only tested in qemu, but I will do some testing on some real hardware in the following days.

FWICT, on this arch the compiler always generates a function prologue somewhere between these lines:

e1a0c00d        mov     ip, sp
e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
e24cb004        sub     fp, ip, #4
e24dd064        sub     sp, sp, #100    ; 0x64<--- local variables
e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
ebf9c2c9        bl      80110364<__gnu_mcount_nc>

....

Every function that follows this pattern (the number of registers pushed and the sp subtraction for the local variables being the only acceptable exception) can be patched with this mechanism. IIRC, only the inline functions and notrace functions do not follow this pattern.

Considering that the function is livepatchable, when the time comes to call ftrace_call, the ftrace_regs_caller is called instead.

Because this arch didn't have a ftrace with regs implementation, the ftrace_regs_caller was added.

This new function adds the regs saving/restoring part, plus the part necessary for the livepatch mechanism to work. After the regs are saved and the r3 is set to contain the sp's value, we're keeping the old pc into r10 in order to be checked later against the new pc.

Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called and the klp_ftrace_handler overwrites the old pc with the new one.

Here comes the tricky part. We're checking if the pc is still the old one, if it is we jump the whole livepatching and go ahead with restoring the saved regs.

If the pc is modified, it means we're livepatching current function and we need to pop all regs from r1 through r12, jump over the next two regs saved on stack (we're not interested in those since we're trying to get the same regs context as it was at the point the function-to-be-patched was called) and put the new pc into r11.

Since r12 contains the sp from when the function just got branched to, we need to set the sp back to that.

Then we need to put the new pc on stack so that when we're popping r11 through pc, we will actually jump to the first instruction from the new function.

We don't need to worry about the returning phase since the epilogue of the new function will take care of that and from there on everything goes back to normal.

The whole advantage of this over adding compiler support is that we're not introducing nops at the beginning of the function. As a matter of fact, we're not changing anything between an image with livepatch and an image without it (except the ftrace_regs_call addition and the livepatch necessary code).

As for the implementation of the ftrace_regs_caller, I still think there might be some unsafe stack handling since I'm getting some build warnings. Those are due to pushing/popping of a list of regs in which the sp resides. I'll try to get around those in a next iteration (if necessary), but first I would like to hear some opinions about this work and if it's worth going forward.

Everything else should be pretty straightforward, so I'll skip explaining that.

Abel Vesa (7):
arm: Add livepatch arch specific code
arm: ftrace: Add call modify mechanism
arm: module: Add apply_relocate_add
arm: Add ftrace with regs support
arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
arm: Add livepatch to build if CONFIG_LIVEPATCH
arm: Add livepatch necessary arch selects into Kconfig

  MAINTAINERS                      |  3 +++
  arch/arm/Kconfig                 |  4 ++++
  arch/arm/include/asm/ftrace.h    |  4 ++++
  arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
  arch/arm/kernel/Makefile         |  1 +
  arch/arm/kernel/entry-ftrace.S   | 49 ++++++++++++++++++++++++++++++++++++++++
  arch/arm/kernel/ftrace.c         | 21 +++++++++++++++++
  arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++
  arch/arm/kernel/module.c         |  9 ++++++++
  9 files changed, 180 insertions(+)

create mode 100644 arch/arm/include/asm/livepatch.h create mode 100644 arch/arm/kernel/livepatch.c

Quoted 61 lines · Abel Vesa

This is just an idea I've been trying out for a while now.

Just in case somebody wants to play with it, this applies to linux-arm/for-next.

Also please note that this was only tested in qemu, but I will do some testing on some real hardware in the following days.

FWICT, on this arch the compiler always generates a function prologue somewhere between these lines:

e1a0c00d        mov     ip, sp
e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
e24cb004        sub     fp, ip, #4
e24dd064        sub     sp, sp, #100    ; 0x64<--- local variables
e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
ebf9c2c9        bl      80110364<__gnu_mcount_nc>

....

Every function that follows this pattern (the number of registers pushed and the sp subtraction for the local variables being the only acceptable exception) can be patched with this mechanism. IIRC, only the inline functions and notrace functions do not follow this pattern.

Considering that the function is livepatchable, when the time comes to call ftrace_call, the ftrace_regs_caller is called instead.

Because this arch didn't have a ftrace with regs implementation, the ftrace_regs_caller was added.

This new function adds the regs saving/restoring part, plus the part necessary for the livepatch mechanism to work. After the regs are saved and the r3 is set to contain the sp's value, we're keeping the old pc into r10 in order to be checked later against the new pc.

Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called and the klp_ftrace_handler overwrites the old pc with the new one.

Here comes the tricky part. We're checking if the pc is still the old one, if it is we jump the whole livepatching and go ahead with restoring the saved regs.

If the pc is modified, it means we're livepatching current function and we need to pop all regs from r1 through r12, jump over the next two regs saved on stack (we're not interested in those since we're trying to get the same regs context as it was at the point the function-to-be-patched was called) and put the new pc into r11.

Since r12 contains the sp from when the function just got branched to, we need to set the sp back to that.

Then we need to put the new pc on stack so that when we're popping r11 through pc, we will actually jump to the first instruction from the new function.

We don't need to worry about the returning phase since the epilogue of the new function will take care of that and from there on everything goes back to normal.

The whole advantage of this over adding compiler support is that we're not introducing nops at the beginning of the function. As a matter of fact, we're not changing anything between an image with livepatch and an image without it (except the ftrace_regs_call addition and the livepatch necessary code).

Quoted 6 lines

As for the implementation of the ftrace_regs_caller, I still think there might be some unsafe stack handling since I'm getting some build warnings. Those are due to pushing/popping of a list of regs in which the sp resides. I'll try to get around those in a next iteration (if necessary), but first I would like to hear some opinions about this work and if it's worth going forward.

Hi, so your idea is that when the pc is modified, we undo the work of the prologue of the old function, and then jump to the first instruction of the new function. But I doubt if we can really undo the work of the prologue correctly ? I don't know about arm, but gcc on arm64 may do some tricky things in prologue. So is there any chance we may restore a wrong context for the new function ?

Thanks.

Quoted 24 lines

Everything else should be pretty straightforward, so I'll skip explaining that.

Abel Vesa (7):
arm: Add livepatch arch specific code
arm: ftrace: Add call modify mechanism
arm: module: Add apply_relocate_add
arm: Add ftrace with regs support
arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
arm: Add livepatch to build if CONFIG_LIVEPATCH
arm: Add livepatch necessary arch selects into Kconfig

 MAINTAINERS                      |  3 +++
 arch/arm/Kconfig                 |  4 ++++
 arch/arm/include/asm/ftrace.h    |  4 ++++
 arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/Makefile         |  1 +
 arch/arm/kernel/entry-ftrace.S   | 49 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/ftrace.c         | 21 +++++++++++++++++
 arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++
 arch/arm/kernel/module.c         |  9 ++++++++
 9 files changed, 180 insertions(+)
 create mode 100644 arch/arm/include/asm/livepatch.h
 create mode 100644 arch/arm/kernel/livepatch.c

This is just an idea I've been trying out for a while now.

Just in case somebody wants to play with it, this applies to linux-arm/for-next.

Also please note that this was only tested in qemu, but I will do some testing on some real hardware in the following days.

FWICT, on this arch the compiler always generates a function prologue somewhere between these lines:

e1a0c00d        mov     ip, sp
e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
e24cb004        sub     fp, ip, #4
e24dd064        sub     sp, sp, #100    ; 0x64 <--- local variables
e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
ebf9c2c9        bl      80110364 <__gnu_mcount_nc>

....

Every function that follows this pattern (the number of registers pushed and the sp subtraction for the local variables being the only acceptable exception) can be patched with this mechanism. IIRC, only the inline functions and notrace functions do not follow this pattern.