live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro()
@ 2020-04-17 14:04 Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 1/9] livepatch: Disallow vmlinux.ko Josh Poimboeuf
                   ` (8 more replies)
  0 siblings, 9 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu

v2:
- add vmlinux.ko check [peterz]
- remove 'klp_object' forward declaration [mbenes]
- use text_mutex [jeyu]
- fix documentation TOC [jeyu]
- fix s390 issues [mbenes]
- upstream kpatch-build now supports this
  (though it's only enabled for Linux >= 5.8)

These patches add simplifications and improvements for some issues Peter
found six months ago, as part of his non-writable text code (W^X)
cleanups.

Highlights:

- Remove the livepatch arch-specific .klp.arch sections, which were used
  to do paravirt patching and alternatives patching for livepatch
  replacement code.

- Add support for jump labels in patched code.

- Remove the last module_disable_ro() usage.

For more background, see this thread:

  https://lkml.kernel.org/r/20191021135312.jbbxsuipxldocdjk@treble

I've tested this with a modified kpatch-build:

  https://github.com/jpoimboe/kpatch/tree/no-klp-arch

(I'm planning to submit a github PR for kpatch-build, once I get
 the updated unit/integration tests sorted out.



Josh Poimboeuf (6):
  livepatch: Disallow vmlinux.ko
  livepatch: Apply vmlinux-specific KLP relocations early
  livepatch: Prevent module-specific KLP rela sections from referencing
    vmlinux symbols
  s390: Change s390_kernel_write() return type to match memcpy()
  livepatch: Remove module_disable_ro() usage
  module: Remove module_disable_ro()

Peter Zijlstra (3):
  livepatch: Remove .klp.arch
  s390/module: Use s390_kernel_write() for late relocations
  x86/module: Use text_poke() for late relocations

 Documentation/livepatch/module-elf-format.rst |  15 +-
 arch/s390/include/asm/uaccess.h               |   2 +-
 arch/s390/kernel/module.c                     | 125 ++++++++++------
 arch/s390/mm/maccess.c                        |   9 +-
 arch/um/kernel/um_arch.c                      |  16 +++
 arch/x86/kernel/Makefile                      |   1 -
 arch/x86/kernel/livepatch.c                   |  53 -------
 arch/x86/kernel/module.c                      |  43 +++++-
 include/linux/livepatch.h                     |  17 ++-
 include/linux/module.h                        |   2 -
 kernel/livepatch/core.c                       | 133 +++++++++++-------
 kernel/module.c                               |  22 +--
 12 files changed, 243 insertions(+), 195 deletions(-)
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.21.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 1/9] livepatch: Disallow vmlinux.ko
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early Josh Poimboeuf
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu, Miroslav Benes

This is purely a theoretical issue, but if there were a module named
vmlinux.ko, the livepatch relocation code wouldn't be able to
distinguish between vmlinux-specific and vmlinux.o-specific KLP
relocations.

If CONFIG_LIVEPATCH is enabled, don't allow a module named vmlinux.ko.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/livepatch/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index c3512e7e0801..40cfac8156fd 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1139,6 +1139,11 @@ int klp_module_coming(struct module *mod)
 	if (WARN_ON(mod->state != MODULE_STATE_COMING))
 		return -EINVAL;
 
+	if (!strcmp(mod->name, "vmlinux")) {
+		pr_err("vmlinux.ko: invalid module name");
+		return -EINVAL;
+	}
+
 	mutex_lock(&klp_mutex);
 	/*
 	 * Each module has to know that klp_module_coming()
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 1/9] livepatch: Disallow vmlinux.ko Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-20 17:57   ` Joe Lawrence
  2020-04-17 14:04 ` [PATCH v2 3/9] livepatch: Remove .klp.arch Josh Poimboeuf
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu

KLP relocations are livepatch-specific relocations which are applied to
a KLP module's text or data.  They exist for two reasons:

  1) Unexported symbols: replacement functions often need to access
     unexported symbols (e.g. static functions), which "normal"
     relocations don't allow.

  2) Late module patching: this is the ability for a KLP module to
     bypass normal module dependencies, such that the KLP module can be
     loaded *before* a to-be-patched module.  This means that
     relocations which need to access symbols in the to-be-patched
     module might need to be applied to the KLP module well after it has
     been loaded.

Non-late-patched KLP relocations are applied from the KLP module's init
function.  That usually works fine, unless the patched code wants to use
alternatives, paravirt patching, jump tables, or some other special
section which needs relocations.  Then we run into ordering issues and
crashes.

In order for those special sections to work properly, the KLP
relocations should be applied *before* the special section init code
runs, such as apply_paravirt(), apply_alternatives(), or
jump_label_apply_nops().

You might think the obvious solution would be to move the KLP relocation
initialization earlier, but it's not necessarily that simple.  The
problem is the above-mentioned late module patching, for which KLP
relocations can get applied well after the KLP module is loaded.

To "fix" this issue in the past, we created .klp.arch sections:

  .klp.arch.{module}..altinstructions
  .klp.arch.{module}..parainstructions

Those sections allow KLP late module patching code to call
apply_paravirt() and apply_alternatives() after the module-specific KLP
relocations (.klp.rela.{module}.{section}) have been applied.

But that has a lot of drawbacks, including code complexity, the need for
arch-specific code, and the (per-arch) danger that we missed some
special section -- for example the __jump_table section which is used
for jump labels.

It turns out there's a simpler and more functional approach.  There are
two kinds of KLP relocation sections:

  1) vmlinux-specific KLP relocation sections

     .klp.rela.vmlinux.{sec}

     These are relocations (applied to the KLP module) which reference
     unexported vmlinux symbols.

  2) module-specific KLP relocation sections

     .klp.rela.{module}.{sec}:

     These are relocations (applied to the KLP module) which reference
     unexported or exported module symbols.

Up until now, these have been treated the same.  However, they're
inherently different.

Because of late module patching, module-specific KLP relocations can be
applied very late, thus they can create the ordering headaches described
above.

But vmlinux-specific KLP relocations don't have that problem.  There's
nothing to prevent them from being applied earlier.  So apply them at
the same time as normal relocations, when the KLP module is being
loaded.

This means that for vmlinux-specific KLP relocations, we no longer have
any ordering issues.  vmlinux-referencing jump labels, alternatives, and
paravirt patching will work automatically, without the need for the
.klp.arch hacks.

All that said, for module-specific KLP relocations, the ordering
problems still exist and we *do* still need .klp.arch.  Or do we?  Stay
tuned.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/linux/livepatch.h | 14 +++++++
 kernel/livepatch/core.c   | 86 ++++++++++++++++++++++++++-------------
 kernel/module.c           |  9 ++--
 3 files changed, 77 insertions(+), 32 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e894e74905f3..85fc23759dc1 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -234,6 +234,11 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
 struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
 struct klp_state *klp_get_prev_state(unsigned long id);
 
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symindex, struct module *pmod,
+			  const char *objname);
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
@@ -242,6 +247,15 @@ static inline bool klp_patch_pending(struct task_struct *task) { return false; }
 static inline void klp_update_patch_state(struct task_struct *task) {}
 static inline void klp_copy_process(struct task_struct *child) {}
 
+static inline
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symindex, struct module *pmod,
+			  const char *objname)
+{
+	return 0;
+}
+
 #endif /* CONFIG_LIVEPATCH */
 
 #endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 40cfac8156fd..5fda3afc0285 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -191,12 +191,12 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	return -EINVAL;
 }
 
-static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
+static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
+			       unsigned int symndx, Elf_Shdr *relasec)
 {
 	int i, cnt, vmlinux, ret;
 	char objname[MODULE_NAME_LEN];
 	char symname[KSYM_NAME_LEN];
-	char *strtab = pmod->core_kallsyms.strtab;
 	Elf_Rela *relas;
 	Elf_Sym *sym;
 	unsigned long sympos, addr;
@@ -216,7 +216,7 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
 	relas = (Elf_Rela *) relasec->sh_addr;
 	/* For each rela in this klp relocation section */
 	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
-		sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
+		sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
 		if (sym->st_shndx != SHN_LIVEPATCH) {
 			pr_err("symbol %s is not marked as a livepatch symbol\n",
 			       strtab + sym->st_name);
@@ -246,23 +246,41 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
 	return 0;
 }
 
-static int klp_write_object_relocations(struct module *pmod,
-					struct klp_object *obj)
+/*
+ * At a high-level, there are two types of klp relocation sections: those which
+ * reference symbols which live in vmlinux; and those which reference symbols
+ * which live in other modules.  This function is called for both types:
+ *
+ * 1) When a klp module itself loads, the module code calls this function to
+ *    write vmlinux-specific klp relocations (.klp.rela.vmlinux.* sections).
+ *    These relocations are written to the klp module text to allow the patched
+ *    code/data to reference unexported vmlinux symbols.  They're written as
+ *    early as possible to ensure that other module init code (.e.g.,
+ *    jump_label_apply_nops) can access any unexported vmlinux symbols which
+ *    might be referenced by the klp module's special sections.
+ *
+ * 2) When a to-be-patched module loads -- or is already loaded when a
+ *    corresponding klp module loads -- klp code calls this function to write
+ *    module-specific klp relocations (.klp.rela.{module}.* sections).  These
+ *    are written to the klp module text to allow the patched code/data to
+ *    reference symbols which live in the to-be-patched module or one of its
+ *    module dependencies.  Exported symbols are supported, in addition to
+ *    unexported symbols, in order to enable late module patching, which allows
+ *    the to-be-patched module to be loaded and patched sometime *after* the
+ *    klp module is loaded.
+ */
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symndx, struct module *pmod,
+			  const char *objname)
 {
 	int i, cnt, ret = 0;
-	const char *objname, *secname;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))
-		return -EINVAL;
-
-	objname = klp_is_module(obj) ? obj->name : "vmlinux";
-
 	/* For each klp relocation section */
-	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
-		sec = pmod->klp_info->sechdrs + i;
-		secname = pmod->klp_info->secstrings + sec->sh_name;
+	for (i = 1; i < ehdr->e_shnum; i++) {
+		sec = sechdrs + i;
 		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
 			continue;
 
@@ -271,24 +289,23 @@ static int klp_write_object_relocations(struct module *pmod,
 		 * See comment in klp_resolve_symbols() for an explanation
 		 * of the selected field width value.
 		 */
-		cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
+		cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
+			     sec_objname);
 		if (cnt != 1) {
 			pr_err("section %s has an incorrectly formatted name\n",
-			       secname);
+			       shstrtab + sec->sh_name);
 			ret = -EINVAL;
 			break;
 		}
 
-		if (strcmp(objname, sec_objname))
+		if (strcmp(objname ? objname : "vmlinux", sec_objname))
 			continue;
 
-		ret = klp_resolve_symbols(sec, pmod);
+		ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec);
 		if (ret)
 			break;
 
-		ret = apply_relocate_add(pmod->klp_info->sechdrs,
-					 pmod->core_kallsyms.strtab,
-					 pmod->klp_info->symndx, i, pmod);
+		ret = apply_relocate_add(sechdrs, strtab, symndx, i, pmod);
 		if (ret)
 			break;
 	}
@@ -736,20 +753,33 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 {
 	struct klp_func *func;
 	int ret;
+	struct klp_modinfo *info = patch->mod->klp_info;
 
 	mutex_lock(&text_mutex);
-
 	module_disable_ro(patch->mod);
-	ret = klp_write_object_relocations(patch->mod, obj);
-	if (ret) {
-		module_enable_ro(patch->mod, true);
-		mutex_unlock(&text_mutex);
-		return ret;
+
+	if (klp_is_module(obj)) {
+		/*
+		 * Only write module-specific relocations here
+		 * (.klp.rela.{module}.*).  vmlinux-specific relocations were
+		 * written earlier during the initialization of the klp module
+		 * itself.
+		 */
+		ret = klp_write_relocations(&info->hdr, info->sechdrs,
+					    info->secstrings,
+					    patch->mod->core_kallsyms.strtab,
+					    info->symndx, patch->mod,
+					    obj->name);
+		if (ret) {
+			module_enable_ro(patch->mod, true);
+			mutex_unlock(&text_mutex);
+			return ret;
+		}
 	}
 
 	arch_klp_init_object_loaded(patch, obj);
-	module_enable_ro(patch->mod, true);
 
+	module_enable_ro(patch->mod, true);
 	mutex_unlock(&text_mutex);
 
 	klp_for_each_func(obj, func) {
diff --git a/kernel/module.c b/kernel/module.c
index 646f1e2330d2..d36ea8a8c3ec 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
-		/* Livepatch relocation sections are applied by livepatch */
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			continue;
-
-		if (info->sechdrs[i].sh_type == SHT_REL)
+			err = klp_write_relocations(info->hdr, info->sechdrs,
+						    info->secstrings,
+						    info->strtab,
+						    info->index.sym, mod, NULL);
+		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
 		else if (info->sechdrs[i].sh_type == SHT_RELA)
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 3/9] livepatch: Remove .klp.arch
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 1/9] livepatch: Disallow vmlinux.ko Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 4/9] livepatch: Prevent module-specific KLP rela sections from referencing vmlinux symbols Josh Poimboeuf
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu

From: Peter Zijlstra <peterz@infradead.org>

After the previous patch, vmlinux-specific KLP relocations are now
applied early during KLP module load.  This means that .klp.arch
sections are no longer needed for *vmlinux-specific* KLP relocations.

One might think they're still needed for *module-specific* KLP
relocations.  If a to-be-patched module is loaded *after* its
corresponding KLP module is loaded, any corresponding KLP relocations
will be delayed until the to-be-patched module is loaded.  If any
special sections (.parainstructions, for example) rely on those
relocations, their initializations (apply_paravirt) need to be done
afterwards.  Thus the apparent need for arch_klp_init_object_loaded()
and its corresponding .klp.arch sections -- it allows some of the
special section initializations to be done at a later time.

But... if you look closer, that dependency between the special sections
and the module-specific KLP relocations doesn't actually exist in
reality.  Looking at the contents of the .altinstructions and
.parainstructions sections, there's not a realistic scenario in which a
KLP module's .altinstructions or .parainstructions section needs to
access a symbol in a to-be-patched module.  It might need to access a
local symbol or even a vmlinux symbol; but not another module's symbol.
When a special section needs to reference a local or vmlinux symbol, a
normal rela can be used instead of a KLP rela.

Since the special section initializations don't actually have any real
dependency on module-specific KLP relocations, .klp.arch and
arch_klp_init_object_loaded() no longer have a reason to exist.  So
remove them.

As Peter said much more succinctly:

  So the reason for .klp.arch was that .klp.rela.* stuff would overwrite
  paravirt instructions. If that happens you're doing it wrong. Those
  RELAs are core kernel, not module, and thus should've happened in
  .rela.* sections at patch-module loading time.

  Reverting this removes the two apply_{paravirt,alternatives}() calls
  from the late patching path, and means we don't have to worry about
  them when removing module_disable_ro().

[ jpoimboe: Rewrote patch description.  Tweaked klp_init_object_loaded()
	    error path. ]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 Documentation/livepatch/module-elf-format.rst | 15 +-----
 arch/x86/kernel/Makefile                      |  1 -
 arch/x86/kernel/livepatch.c                   | 53 -------------------
 include/linux/livepatch.h                     |  3 --
 kernel/livepatch/core.c                       | 27 ++++------
 5 files changed, 11 insertions(+), 88 deletions(-)
 delete mode 100644 arch/x86/kernel/livepatch.c

diff --git a/Documentation/livepatch/module-elf-format.rst b/Documentation/livepatch/module-elf-format.rst
index 2a591e6f8e6c..8c6b894c4661 100644
--- a/Documentation/livepatch/module-elf-format.rst
+++ b/Documentation/livepatch/module-elf-format.rst
@@ -14,8 +14,7 @@ This document outlines the Elf format requirements that livepatch modules must f
    4. Livepatch symbols
       4.1 A livepatch module's symbol table
       4.2 Livepatch symbol format
-   5. Architecture-specific sections
-   6. Symbol table and Elf section access
+   5. Symbol table and Elf section access
 
 1. Background and motivation
 ============================
@@ -298,17 +297,7 @@ Examples:
   Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
   "OS" means OS-specific.
 
-5. Architecture-specific sections
-=================================
-Architectures may override arch_klp_init_object_loaded() to perform
-additional arch-specific tasks when a target module loads, such as applying
-arch-specific sections. On x86 for example, we must apply per-object
-.altinstructions and .parainstructions sections when a target module loads.
-These sections must be prefixed with ".klp.arch.$objname." so that they can
-be easily identified when iterating through a patch module's Elf sections
-(See arch/x86/kernel/livepatch.c for a complete example).
-
-6. Symbol table and Elf section access
+5. Symbol table and Elf section access
 ======================================
 A livepatch module's symbol table is accessible through module->symtab.
 
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index d6d61c4455fa..fc8834342516 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -94,7 +94,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-y				+= apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
-obj-$(CONFIG_LIVEPATCH)	+= livepatch.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace_$(BITS).o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
deleted file mode 100644
index 6a68e41206e7..000000000000
--- a/arch/x86/kernel/livepatch.c
+++ /dev/null
@@ -1,53 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * livepatch.c - x86-specific Kernel Live Patching Core
- */
-
-#include <linux/module.h>
-#include <linux/kallsyms.h>
-#include <linux/livepatch.h>
-#include <asm/text-patching.h>
-
-/* Apply per-object alternatives. Based on x86 module_finalize() */
-void arch_klp_init_object_loaded(struct klp_patch *patch,
-				 struct klp_object *obj)
-{
-	int cnt;
-	struct klp_modinfo *info;
-	Elf_Shdr *s, *alt = NULL, *para = NULL;
-	void *aseg, *pseg;
-	const char *objname;
-	char sec_objname[MODULE_NAME_LEN];
-	char secname[KSYM_NAME_LEN];
-
-	info = patch->mod->klp_info;
-	objname = obj->name ? obj->name : "vmlinux";
-
-	/* See livepatch core code for BUILD_BUG_ON() explanation */
-	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
-
-	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
-		/* Apply per-object .klp.arch sections */
-		cnt = sscanf(info->secstrings + s->sh_name,
-			     ".klp.arch.%55[^.].%127s",
-			     sec_objname, secname);
-		if (cnt != 2)
-			continue;
-		if (strcmp(sec_objname, objname))
-			continue;
-		if (!strcmp(".altinstructions", secname))
-			alt = s;
-		if (!strcmp(".parainstructions", secname))
-			para = s;
-	}
-
-	if (alt) {
-		aseg = (void *) alt->sh_addr;
-		apply_alternatives(aseg, aseg + alt->sh_size);
-	}
-
-	if (para) {
-		pseg = (void *) para->sh_addr;
-		apply_paravirt(pseg, pseg + para->sh_size);
-	}
-}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 85fc23759dc1..533359e48c39 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -195,9 +195,6 @@ struct klp_patch {
 
 int klp_enable_patch(struct klp_patch *);
 
-void arch_klp_init_object_loaded(struct klp_patch *patch,
-				 struct klp_object *obj);
-
 /* Called from the module loader during module coming/going states */
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5fda3afc0285..9d865b08d0b0 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -741,12 +741,6 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 			   func->old_sympos ? func->old_sympos : 1);
 }
 
-/* Arches may override this to finish any remaining arch-specific tasks */
-void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
-					struct klp_object *obj)
-{
-}
-
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
@@ -755,10 +749,11 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	int ret;
 	struct klp_modinfo *info = patch->mod->klp_info;
 
-	mutex_lock(&text_mutex);
-	module_disable_ro(patch->mod);
-
 	if (klp_is_module(obj)) {
+
+		mutex_lock(&text_mutex);
+		module_disable_ro(patch->mod);
+
 		/*
 		 * Only write module-specific relocations here
 		 * (.klp.rela.{module}.*).  vmlinux-specific relocations were
@@ -770,17 +765,13 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 					    patch->mod->core_kallsyms.strtab,
 					    info->symndx, patch->mod,
 					    obj->name);
-		if (ret) {
-			module_enable_ro(patch->mod, true);
-			mutex_unlock(&text_mutex);
-			return ret;
-		}
-	}
 
-	arch_klp_init_object_loaded(patch, obj);
+		module_enable_ro(patch->mod, true);
+		mutex_unlock(&text_mutex);
 
-	module_enable_ro(patch->mod, true);
-	mutex_unlock(&text_mutex);
+		if (ret)
+			return ret;
+	}
 
 	klp_for_each_func(obj, func) {
 		ret = klp_find_object_symbol(obj->name, func->old_name,
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 4/9] livepatch: Prevent module-specific KLP rela sections from referencing vmlinux symbols
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
                   ` (2 preceding siblings ...)
  2020-04-17 14:04 ` [PATCH v2 3/9] livepatch: Remove .klp.arch Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 5/9] s390: Change s390_kernel_write() return type to match memcpy() Josh Poimboeuf
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu

Prevent module-specific KLP rela sections from referencing vmlinux
symbols.  This helps prevent ordering issues with module special section
initializations.  Presumably such symbols are exported and normal relas
can be used instead.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/livepatch/core.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 9d865b08d0b0..a090185e8689 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -192,17 +192,20 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 }
 
 static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
-			       unsigned int symndx, Elf_Shdr *relasec)
+			       unsigned int symndx, Elf_Shdr *relasec,
+			       const char *sec_objname)
 {
-	int i, cnt, vmlinux, ret;
-	char objname[MODULE_NAME_LEN];
-	char symname[KSYM_NAME_LEN];
+	int i, cnt, ret;
+	char sym_objname[MODULE_NAME_LEN];
+	char sym_name[KSYM_NAME_LEN];
 	Elf_Rela *relas;
 	Elf_Sym *sym;
 	unsigned long sympos, addr;
+	bool sym_vmlinux;
+	bool sec_vmlinux = !strcmp(sec_objname, "vmlinux");
 
 	/*
-	 * Since the field widths for objname and symname in the sscanf()
+	 * Since the field widths for sym_objname and sym_name in the sscanf()
 	 * call are hard-coded and correspond to MODULE_NAME_LEN and
 	 * KSYM_NAME_LEN respectively, we must make sure that MODULE_NAME_LEN
 	 * and KSYM_NAME_LEN have the values we expect them to have.
@@ -223,20 +226,33 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
 			return -EINVAL;
 		}
 
-		/* Format: .klp.sym.objname.symname,sympos */
+		/* Format: .klp.sym.sym_objname.sym_name,sympos */
 		cnt = sscanf(strtab + sym->st_name,
 			     ".klp.sym.%55[^.].%127[^,],%lu",
-			     objname, symname, &sympos);
+			     sym_objname, sym_name, &sympos);
 		if (cnt != 3) {
 			pr_err("symbol %s has an incorrectly formatted name\n",
 			       strtab + sym->st_name);
 			return -EINVAL;
 		}
 
+		sym_vmlinux = !strcmp(sym_objname, "vmlinux");
+
+		/*
+		 * Prevent module-specific KLP rela sections from referencing
+		 * vmlinux symbols.  This helps prevent ordering issues with
+		 * module special section initializations.  Presumably such
+		 * symbols are exported and normal relas can be used instead.
+		 */
+		if (!sec_vmlinux && sym_vmlinux) {
+			pr_err("invalid access to vmlinux symbol '%s' from module-specific livepatch relocation section",
+			       sym_name);
+			return -EINVAL;
+		}
+
 		/* klp_find_object_symbol() treats a NULL objname as vmlinux */
-		vmlinux = !strcmp(objname, "vmlinux");
-		ret = klp_find_object_symbol(vmlinux ? NULL : objname,
-					     symname, sympos, &addr);
+		ret = klp_find_object_symbol(sym_vmlinux ? NULL : sym_objname,
+					     sym_name, sympos, &addr);
 		if (ret)
 			return ret;
 
@@ -301,7 +317,8 @@ int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 		if (strcmp(objname ? objname : "vmlinux", sec_objname))
 			continue;
 
-		ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec);
+		ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
+					  sec_objname);
 		if (ret)
 			break;
 
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 5/9] s390: Change s390_kernel_write() return type to match memcpy()
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
                   ` (3 preceding siblings ...)
  2020-04-17 14:04 ` [PATCH v2 4/9] livepatch: Prevent module-specific KLP rela sections from referencing vmlinux symbols Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations Josh Poimboeuf
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching
  Cc: linux-kernel, Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens

s390_kernel_write()'s function type is almost identical to memcpy().
Change its return type to "void *" so they can be used interchangeably.

Cc: linux-s390@vger.kernel.org
Cc: heiko.carstens@de.ibm.com
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/s390/include/asm/uaccess.h | 2 +-
 arch/s390/mm/maccess.c          | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index a470f1fa9f2a..324438889fe1 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -276,6 +276,6 @@ static inline unsigned long __must_check clear_user(void __user *to, unsigned lo
 }
 
 int copy_to_user_real(void __user *dest, void *src, unsigned long count);
-void s390_kernel_write(void *dst, const void *src, size_t size);
+void *s390_kernel_write(void *dst, const void *src, size_t size);
 
 #endif /* __S390_UACCESS_H */
diff --git a/arch/s390/mm/maccess.c b/arch/s390/mm/maccess.c
index de7ca4b6718f..22a0be655f27 100644
--- a/arch/s390/mm/maccess.c
+++ b/arch/s390/mm/maccess.c
@@ -55,19 +55,22 @@ static notrace long s390_kernel_write_odd(void *dst, const void *src, size_t siz
  */
 static DEFINE_SPINLOCK(s390_kernel_write_lock);
 
-void notrace s390_kernel_write(void *dst, const void *src, size_t size)
+notrace void *s390_kernel_write(void *dst, const void *src, size_t size)
 {
+	void *tmp = dst;
 	unsigned long flags;
 	long copied;
 
 	spin_lock_irqsave(&s390_kernel_write_lock, flags);
 	while (size) {
-		copied = s390_kernel_write_odd(dst, src, size);
-		dst += copied;
+		copied = s390_kernel_write_odd(tmp, src, size);
+		tmp += copied;
 		src += copied;
 		size -= copied;
 	}
 	spin_unlock_irqrestore(&s390_kernel_write_lock, flags);
+
+	return dst;
 }
 
 static int __no_sanitize_address __memcpy_real(void *dest, void *src, size_t count)
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
                   ` (4 preceding siblings ...)
  2020-04-17 14:04 ` [PATCH v2 5/9] s390: Change s390_kernel_write() return type to match memcpy() Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-22 12:28   ` Miroslav Benes
  2020-04-22 14:40   ` Gerald Schaefer
  2020-04-17 14:04 ` [PATCH v2 7/9] x86/module: Use text_poke() " Josh Poimboeuf
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching
  Cc: linux-kernel, Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens

From: Peter Zijlstra <peterz@infradead.org>

Because of late module patching, a livepatch module needs to be able to
apply some of its relocations well after it has been loaded.  Instead of
playing games with module_{dis,en}able_ro(), use existing text poking
mechanisms to apply relocations after module loading.

So far only x86, s390 and Power have HAVE_LIVEPATCH but only the first
two also have STRICT_MODULE_RWX.

This will allow removal of the last module_disable_ro() usage in
livepatch.  The ultimate goal is to completely disallow making
executable mappings writable.

Also, for the late patching case, use text_mutex, which is supposed to
be held for all runtime text patching operations.

[ jpoimboe: Split up patches.  Use mod state to determine whether
	    memcpy() can be used.  Add text_mutex.  Make it build. ]

Cc: linux-s390@vger.kernel.org
Cc: heiko.carstens@de.ibm.com
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/s390/kernel/module.c | 125 ++++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 46 deletions(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index ba8f19bb438b..2798329ebb74 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -19,6 +19,7 @@
 #include <linux/kasan.h>
 #include <linux/moduleloader.h>
 #include <linux/bug.h>
+#include <linux/memory.h>
 #include <asm/alternative.h>
 #include <asm/nospec-branch.h>
 #include <asm/facility.h>
@@ -174,10 +175,12 @@ int module_frob_arch_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 }
 
 static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
-			   int sign, int bits, int shift)
+			   int sign, int bits, int shift,
+			   void *(*write)(void *dest, const void *src, size_t len))
 {
 	unsigned long umax;
 	long min, max;
+	void *dest = (void *)loc;
 
 	if (val & ((1UL << shift) - 1))
 		return -ENOEXEC;
@@ -194,26 +197,33 @@ static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
 			return -ENOEXEC;
 	}
 
-	if (bits == 8)
-		*(unsigned char *) loc = val;
-	else if (bits == 12)
-		*(unsigned short *) loc = (val & 0xfff) |
+	if (bits == 8) {
+		unsigned char tmp = val;
+		write(dest, &tmp, 1);
+	} else if (bits == 12) {
+		unsigned short tmp = (val & 0xfff) |
 			(*(unsigned short *) loc & 0xf000);
-	else if (bits == 16)
-		*(unsigned short *) loc = val;
-	else if (bits == 20)
-		*(unsigned int *) loc = (val & 0xfff) << 16 |
-			(val & 0xff000) >> 4 |
-			(*(unsigned int *) loc & 0xf00000ff);
-	else if (bits == 32)
-		*(unsigned int *) loc = val;
-	else if (bits == 64)
-		*(unsigned long *) loc = val;
+		write(dest, &tmp, 2);
+	} else if (bits == 16) {
+		unsigned short tmp = val;
+		write(dest, &tmp, 2);
+	} else if (bits == 20) {
+		unsigned int tmp = (val & 0xfff) << 16 |
+			(val & 0xff000) >> 4 | (*(unsigned int *) loc & 0xf00000ff);
+		write(dest, &tmp, 4);
+	} else if (bits == 32) {
+		unsigned int tmp = val;
+		write(dest, &tmp, 4);
+	} else if (bits == 64) {
+		unsigned long tmp = val;
+		write(dest, &tmp, 8);
+	}
 	return 0;
 }
 
 static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
-		      const char *strtab, struct module *me)
+		      const char *strtab, struct module *me,
+		      void *(*write)(void *dest, const void *src, size_t len))
 {
 	struct mod_arch_syminfo *info;
 	Elf_Addr loc, val;
@@ -241,17 +251,17 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_64:		/* Direct 64 bit.  */
 		val += rela->r_addend;
 		if (r_type == R_390_8)
-			rc = apply_rela_bits(loc, val, 0, 8, 0);
+			rc = apply_rela_bits(loc, val, 0, 8, 0, write);
 		else if (r_type == R_390_12)
-			rc = apply_rela_bits(loc, val, 0, 12, 0);
+			rc = apply_rela_bits(loc, val, 0, 12, 0, write);
 		else if (r_type == R_390_16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_20)
-			rc = apply_rela_bits(loc, val, 1, 20, 0);
+			rc = apply_rela_bits(loc, val, 1, 20, 0, write);
 		else if (r_type == R_390_32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		break;
 	case R_390_PC16:	/* PC relative 16 bit.  */
 	case R_390_PC16DBL:	/* PC relative 16 bit shifted by 1.  */
@@ -260,15 +270,15 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_PC64:	/* PC relative 64 bit.	*/
 		val += rela->r_addend - loc;
 		if (r_type == R_390_PC16)
-			rc = apply_rela_bits(loc, val, 1, 16, 0);
+			rc = apply_rela_bits(loc, val, 1, 16, 0, write);
 		else if (r_type == R_390_PC16DBL)
-			rc = apply_rela_bits(loc, val, 1, 16, 1);
+			rc = apply_rela_bits(loc, val, 1, 16, 1, write);
 		else if (r_type == R_390_PC32DBL)
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		else if (r_type == R_390_PC32)
-			rc = apply_rela_bits(loc, val, 1, 32, 0);
+			rc = apply_rela_bits(loc, val, 1, 32, 0, write);
 		else if (r_type == R_390_PC64)
-			rc = apply_rela_bits(loc, val, 1, 64, 0);
+			rc = apply_rela_bits(loc, val, 1, 64, 0, write);
 		break;
 	case R_390_GOT12:	/* 12 bit GOT offset.  */
 	case R_390_GOT16:	/* 16 bit GOT offset.  */
@@ -293,23 +303,23 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 		val = info->got_offset + rela->r_addend;
 		if (r_type == R_390_GOT12 ||
 		    r_type == R_390_GOTPLT12)
-			rc = apply_rela_bits(loc, val, 0, 12, 0);
+			rc = apply_rela_bits(loc, val, 0, 12, 0, write);
 		else if (r_type == R_390_GOT16 ||
 			 r_type == R_390_GOTPLT16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_GOT20 ||
 			 r_type == R_390_GOTPLT20)
-			rc = apply_rela_bits(loc, val, 1, 20, 0);
+			rc = apply_rela_bits(loc, val, 1, 20, 0, write);
 		else if (r_type == R_390_GOT32 ||
 			 r_type == R_390_GOTPLT32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_GOT64 ||
 			 r_type == R_390_GOTPLT64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		else if (r_type == R_390_GOTENT ||
 			 r_type == R_390_GOTPLTENT) {
 			val += (Elf_Addr) me->core_layout.base - loc;
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		}
 		break;
 	case R_390_PLT16DBL:	/* 16 bit PC rel. PLT shifted by 1.  */
@@ -357,17 +367,17 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 			val += rela->r_addend - loc;
 		}
 		if (r_type == R_390_PLT16DBL)
-			rc = apply_rela_bits(loc, val, 1, 16, 1);
+			rc = apply_rela_bits(loc, val, 1, 16, 1, write);
 		else if (r_type == R_390_PLTOFF16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_PLT32DBL)
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		else if (r_type == R_390_PLT32 ||
 			 r_type == R_390_PLTOFF32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_PLT64 ||
 			 r_type == R_390_PLTOFF64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		break;
 	case R_390_GOTOFF16:	/* 16 bit offset to GOT.  */
 	case R_390_GOTOFF32:	/* 32 bit offset to GOT.  */
@@ -375,20 +385,20 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 		val = val + rela->r_addend -
 			((Elf_Addr) me->core_layout.base + me->arch.got_offset);
 		if (r_type == R_390_GOTOFF16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_GOTOFF32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_GOTOFF64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		break;
 	case R_390_GOTPC:	/* 32 bit PC relative offset to GOT. */
 	case R_390_GOTPCDBL:	/* 32 bit PC rel. off. to GOT shifted by 1. */
 		val = (Elf_Addr) me->core_layout.base + me->arch.got_offset +
 			rela->r_addend - loc;
 		if (r_type == R_390_GOTPC)
-			rc = apply_rela_bits(loc, val, 1, 32, 0);
+			rc = apply_rela_bits(loc, val, 1, 32, 0, write);
 		else if (r_type == R_390_GOTPCDBL)
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		break;
 	case R_390_COPY:
 	case R_390_GLOB_DAT:	/* Create GOT entry.  */
@@ -412,9 +422,10 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	return 0;
 }
 
-int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
+static int __apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		       unsigned int symindex, unsigned int relsec,
-		       struct module *me)
+		       struct module *me,
+		       void *(*write)(void *dest, const void *src, size_t len))
 {
 	Elf_Addr base;
 	Elf_Sym *symtab;
@@ -430,13 +441,35 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	n = sechdrs[relsec].sh_size / sizeof(Elf_Rela);
 
 	for (i = 0; i < n; i++, rela++) {
-		rc = apply_rela(rela, base, symtab, strtab, me);
+		rc = apply_rela(rela, base, symtab, strtab, me, write);
 		if (rc)
 			return rc;
 	}
 	return 0;
 }
 
+int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
+		       unsigned int symindex, unsigned int relsec,
+		       struct module *me)
+{
+	int ret;
+	bool early = me->state == MODULE_STATE_UNFORMED;
+	void *(*write)(void *, const void *, size_t) = memcpy;
+
+	if (!early) {
+		write = s390_kernel_write;
+		mutex_lock(&text_mutex);
+	}
+
+	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				   write);
+
+	if (!early)
+		mutex_unlock(&text_mutex);
+
+	return ret;
+}
+
 int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 7/9] x86/module: Use text_poke() for late relocations
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
                   ` (5 preceding siblings ...)
  2020-04-17 14:04 ` [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-17 14:29   ` Peter Zijlstra
  2020-04-17 14:04 ` [PATCH v2 8/9] livepatch: Remove module_disable_ro() usage Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 9/9] module: Remove module_disable_ro() Josh Poimboeuf
  8 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu, x86

From: Peter Zijlstra <peterz@infradead.org>

Because of late module patching, a livepatch module needs to be able to
apply some of its relocations well after it has been loaded.  Instead of
playing games with module_{dis,en}able_ro(), use existing text poking
mechanisms to apply relocations after module loading.

So far only x86, s390 and Power have HAVE_LIVEPATCH but only the first
two also have STRICT_MODULE_RWX.

This will allow removal of the last module_disable_ro() usage in
livepatch.  The ultimate goal is to completely disallow making
executable mappings writable.

Also, for the late patching case, use text_mutex, which is supposed to
be held for all runtime text patching operations.

[ jpoimboe: Split up patches.  Use mod state to determine whether
	    memcpy() can be used.  Implement text_poke() for UML.  Add
	    text_mutex. ]

Cc: x86@kernel.org
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/um/kernel/um_arch.c | 16 +++++++++++++++
 arch/x86/kernel/module.c | 43 +++++++++++++++++++++++++++++++++-------
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index 0f40eccbd759..375ab720e4aa 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -362,3 +362,19 @@ void __init check_bugs(void)
 void apply_alternatives(struct alt_instr *start, struct alt_instr *end)
 {
 }
+
+void *text_poke(void *addr, const void *opcode, size_t len)
+{
+	/*
+	 * In UML, the only reference to this function is in
+	 * apply_relocate_add(), which shouldn't ever actually call this
+	 * because UML doesn't have live patching.
+	 */
+	WARN_ON(1);
+
+	return memcpy(addr, opcode, len);
+}
+
+void text_poke_sync(void)
+{
+}
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index d5c72cb877b3..2a997afa04c6 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -18,6 +18,7 @@
 #include <linux/gfp.h>
 #include <linux/jump_label.h>
 #include <linux/random.h>
+#include <linux/memory.h>
 
 #include <asm/text-patching.h>
 #include <asm/page.h>
@@ -126,11 +127,12 @@ int apply_relocate(Elf32_Shdr *sechdrs,
 	return 0;
 }
 #else /*X86_64*/
-int apply_relocate_add(Elf64_Shdr *sechdrs,
+static int __apply_relocate_add(Elf64_Shdr *sechdrs,
 		   const char *strtab,
 		   unsigned int symindex,
 		   unsigned int relsec,
-		   struct module *me)
+		   struct module *me,
+		   void *(*write)(void *dest, const void *src, size_t len))
 {
 	unsigned int i;
 	Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr;
@@ -162,19 +164,19 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 		case R_X86_64_64:
 			if (*(u64 *)loc != 0)
 				goto invalid_relocation;
-			*(u64 *)loc = val;
+			write(loc, &val, 8);
 			break;
 		case R_X86_64_32:
 			if (*(u32 *)loc != 0)
 				goto invalid_relocation;
-			*(u32 *)loc = val;
+			write(loc, &val, 4);
 			if (val != *(u32 *)loc)
 				goto overflow;
 			break;
 		case R_X86_64_32S:
 			if (*(s32 *)loc != 0)
 				goto invalid_relocation;
-			*(s32 *)loc = val;
+			write(loc, &val, 4);
 			if ((s64)val != *(s32 *)loc)
 				goto overflow;
 			break;
@@ -183,7 +185,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 			if (*(u32 *)loc != 0)
 				goto invalid_relocation;
 			val -= (u64)loc;
-			*(u32 *)loc = val;
+			write(loc, &val, 4);
 #if 0
 			if ((s64)val != *(s32 *)loc)
 				goto overflow;
@@ -193,7 +195,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 			if (*(u64 *)loc != 0)
 				goto invalid_relocation;
 			val -= (u64)loc;
-			*(u64 *)loc = val;
+			write(loc, &val, 8);
 			break;
 		default:
 			pr_err("%s: Unknown rela relocation: %llu\n",
@@ -215,6 +217,33 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	       me->name);
 	return -ENOEXEC;
 }
+
+int apply_relocate_add(Elf64_Shdr *sechdrs,
+		   const char *strtab,
+		   unsigned int symindex,
+		   unsigned int relsec,
+		   struct module *me)
+{
+	int ret;
+	bool early = me->state == MODULE_STATE_UNFORMED;
+	void *(*write)(void *, const void *, size_t) = memcpy;
+
+	if (!early) {
+		write = text_poke;
+		mutex_lock(&text_mutex);
+	}
+
+	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				   write);
+
+	if (!early) {
+		mutex_unlock(&text_mutex);
+		text_poke_sync();
+	}
+
+	return ret;
+}
+
 #endif
 
 int module_finalize(const Elf_Ehdr *hdr,
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 8/9] livepatch: Remove module_disable_ro() usage
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
                   ` (6 preceding siblings ...)
  2020-04-17 14:04 ` [PATCH v2 7/9] x86/module: Use text_poke() " Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  2020-04-17 14:04 ` [PATCH v2 9/9] module: Remove module_disable_ro() Josh Poimboeuf
  8 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu

With arch_klp_init_object_loaded() gone, and apply_relocate_add() now
using text_poke(), livepatch no longer needs to use module_disable_ro().

The text_mutex usage can also be removed -- its purpose was to protect
against module permission change races.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/livepatch/core.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a090185e8689..3ff886b911ae 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -767,10 +767,6 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	struct klp_modinfo *info = patch->mod->klp_info;
 
 	if (klp_is_module(obj)) {
-
-		mutex_lock(&text_mutex);
-		module_disable_ro(patch->mod);
-
 		/*
 		 * Only write module-specific relocations here
 		 * (.klp.rela.{module}.*).  vmlinux-specific relocations were
@@ -782,10 +778,6 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 					    patch->mod->core_kallsyms.strtab,
 					    info->symndx, patch->mod,
 					    obj->name);
-
-		module_enable_ro(patch->mod, true);
-		mutex_unlock(&text_mutex);
-
 		if (ret)
 			return ret;
 	}
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 9/9] module: Remove module_disable_ro()
  2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
                   ` (7 preceding siblings ...)
  2020-04-17 14:04 ` [PATCH v2 8/9] livepatch: Remove module_disable_ro() usage Josh Poimboeuf
@ 2020-04-17 14:04 ` Josh Poimboeuf
  8 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:04 UTC (permalink / raw)
  To: live-patching; +Cc: linux-kernel, Peter Zijlstra, Jessica Yu

module_disable_ro() has no more users.  Remove it.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/linux/module.h |  2 --
 kernel/module.c        | 13 -------------
 2 files changed, 15 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 1ad393e62bef..e4ef7b36feda 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -860,10 +860,8 @@ extern int module_sysfs_initialized;
 
 #ifdef CONFIG_STRICT_MODULE_RWX
 extern void module_enable_ro(const struct module *mod, bool after_init);
-extern void module_disable_ro(const struct module *mod);
 #else
 static inline void module_enable_ro(const struct module *mod, bool after_init) { }
-static inline void module_disable_ro(const struct module *mod) { }
 #endif
 
 #ifdef CONFIG_GENERIC_BUG
diff --git a/kernel/module.c b/kernel/module.c
index d36ea8a8c3ec..b1d30ad67e82 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1997,19 +1997,6 @@ static void frob_writable_data(const struct module_layout *layout,
 		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
 }
 
-/* livepatching wants to disable read-only so it can frob module. */
-void module_disable_ro(const struct module *mod)
-{
-	if (!rodata_enabled)
-		return;
-
-	frob_text(&mod->core_layout, set_memory_rw);
-	frob_rodata(&mod->core_layout, set_memory_rw);
-	frob_ro_after_init(&mod->core_layout, set_memory_rw);
-	frob_text(&mod->init_layout, set_memory_rw);
-	frob_rodata(&mod->init_layout, set_memory_rw);
-}
-
 void module_enable_ro(const struct module *mod, bool after_init)
 {
 	if (!rodata_enabled)
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 7/9] x86/module: Use text_poke() for late relocations
  2020-04-17 14:04 ` [PATCH v2 7/9] x86/module: Use text_poke() " Josh Poimboeuf
@ 2020-04-17 14:29   ` Peter Zijlstra
  2020-04-17 14:51     ` Josh Poimboeuf
  0 siblings, 1 reply; 38+ messages in thread
From: Peter Zijlstra @ 2020-04-17 14:29 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: live-patching, linux-kernel, Jessica Yu, x86

On Fri, Apr 17, 2020 at 09:04:32AM -0500, Josh Poimboeuf wrote:
> +int apply_relocate_add(Elf64_Shdr *sechdrs,
> +		   const char *strtab,
> +		   unsigned int symindex,
> +		   unsigned int relsec,
> +		   struct module *me)
> +{
> +	int ret;
> +	bool early = me->state == MODULE_STATE_UNFORMED;
> +	void *(*write)(void *, const void *, size_t) = memcpy;
> +
> +	if (!early) {
> +		write = text_poke;
> +		mutex_lock(&text_mutex);
> +	}
> +
> +	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> +				   write);
> +
> +	if (!early) {
> +		mutex_unlock(&text_mutex);
> +		text_poke_sync();

I'm thinking text_poke_sync() wants to be inside text_mutex. Although
given that nothing should be running that text, it really doesn't
matter.

> +	}
> +
> +	return ret;
> +}
> +
>  #endif
>  
>  int module_finalize(const Elf_Ehdr *hdr,
> -- 
> 2.21.1
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 7/9] x86/module: Use text_poke() for late relocations
  2020-04-17 14:29   ` Peter Zijlstra
@ 2020-04-17 14:51     ` Josh Poimboeuf
  0 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-17 14:51 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: live-patching, linux-kernel, Jessica Yu, x86

On Fri, Apr 17, 2020 at 04:29:44PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 17, 2020 at 09:04:32AM -0500, Josh Poimboeuf wrote:
> > +int apply_relocate_add(Elf64_Shdr *sechdrs,
> > +		   const char *strtab,
> > +		   unsigned int symindex,
> > +		   unsigned int relsec,
> > +		   struct module *me)
> > +{
> > +	int ret;
> > +	bool early = me->state == MODULE_STATE_UNFORMED;
> > +	void *(*write)(void *, const void *, size_t) = memcpy;
> > +
> > +	if (!early) {
> > +		write = text_poke;
> > +		mutex_lock(&text_mutex);
> > +	}
> > +
> > +	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > +				   write);
> > +
> > +	if (!early) {
> > +		mutex_unlock(&text_mutex);
> > +		text_poke_sync();
> 
> I'm thinking text_poke_sync() wants to be inside text_mutex. Although
> given that nothing should be running that text, it really doesn't
> matter.

Yeah, makes sense.

diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 2a997afa04c6..23c95a53d20e 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -237,8 +237,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 				   write);
 
 	if (!early) {
-		mutex_unlock(&text_mutex);
 		text_poke_sync();
+		mutex_unlock(&text_mutex);
 	}
 
 	return ret;


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-17 14:04 ` [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early Josh Poimboeuf
@ 2020-04-20 17:57   ` Joe Lawrence
  2020-04-20 18:25     ` Josh Poimboeuf
  2020-04-21 11:54     ` Miroslav Benes
  0 siblings, 2 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-20 17:57 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> 
> [ ... snip ... ]
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 40cfac8156fd..5fda3afc0285 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> 
> [ ... snip ... ]
> 
> +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> +			  const char *shstrtab, const char *strtab,
> +			  unsigned int symndx, struct module *pmod,
> +			  const char *objname)
>  {
>  	int i, cnt, ret = 0;
> -	const char *objname, *secname;
>  	char sec_objname[MODULE_NAME_LEN];
>  	Elf_Shdr *sec;
>  
> -	if (WARN_ON(!klp_is_object_loaded(obj)))
> -		return -EINVAL;
> -
> -	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> -
>  	/* For each klp relocation section */
> -	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> -		sec = pmod->klp_info->sechdrs + i;
> -		secname = pmod->klp_info->secstrings + sec->sh_name;
> +	for (i = 1; i < ehdr->e_shnum; i++) {
> +		sec = sechdrs + i;

Hi Josh, minor bug:

Note the for loop through the section headers in
klp_write_relocations(), but its calling function ...

> [ ... snip ... ]
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 646f1e2330d2..d36ea8a8c3ec 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>  			continue;
>  
> -		/* Livepatch relocation sections are applied by livepatch */
>  		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> -			continue;
> -
> -		if (info->sechdrs[i].sh_type == SHT_REL)
> +			err = klp_write_relocations(info->hdr, info->sechdrs,
> +						    info->secstrings,
> +						    info->strtab,
> +						    info->index.sym, mod, NULL);
> +		else if (info->sechdrs[i].sh_type == SHT_REL)
>  			err = apply_relocate(info->sechdrs, info->strtab,
>  					     info->index.sym, i, mod);
>  		else if (info->sechdrs[i].sh_type == SHT_RELA)

... apply_relocations() is also iterating over the section headers (the
diff context doesn't show it here, but i is an incrementing index over
sechdrs[]).

So if there is more than one KLP relocation section, we'll process them
multiple times.  At least the x86 relocation code will detect this and
fail the module load with an invalid relocation (existing value not
zero).

-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 17:57   ` Joe Lawrence
@ 2020-04-20 18:25     ` Josh Poimboeuf
  2020-04-20 19:01       ` Joe Lawrence
  2020-04-21 11:54     ` Miroslav Benes
  1 sibling, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-20 18:25 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, Apr 20, 2020 at 01:57:51PM -0400, Joe Lawrence wrote:
> On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> > 
> > [ ... snip ... ]
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 40cfac8156fd..5fda3afc0285 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > 
> > [ ... snip ... ]
> > 
> > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > +			  const char *shstrtab, const char *strtab,
> > +			  unsigned int symndx, struct module *pmod,
> > +			  const char *objname)
> >  {
> >  	int i, cnt, ret = 0;
> > -	const char *objname, *secname;
> >  	char sec_objname[MODULE_NAME_LEN];
> >  	Elf_Shdr *sec;
> >  
> > -	if (WARN_ON(!klp_is_object_loaded(obj)))
> > -		return -EINVAL;
> > -
> > -	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > -
> >  	/* For each klp relocation section */
> > -	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > -		sec = pmod->klp_info->sechdrs + i;
> > -		secname = pmod->klp_info->secstrings + sec->sh_name;
> > +	for (i = 1; i < ehdr->e_shnum; i++) {
> > +		sec = sechdrs + i;
> 
> Hi Josh, minor bug:
> 
> Note the for loop through the section headers in
> klp_write_relocations(), but its calling function ...
> 
> > [ ... snip ... ]
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 646f1e2330d2..d36ea8a8c3ec 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> >  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> >  			continue;
> >  
> > -		/* Livepatch relocation sections are applied by livepatch */
> >  		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > -			continue;
> > -
> > -		if (info->sechdrs[i].sh_type == SHT_REL)
> > +			err = klp_write_relocations(info->hdr, info->sechdrs,
> > +						    info->secstrings,
> > +						    info->strtab,
> > +						    info->index.sym, mod, NULL);
> > +		else if (info->sechdrs[i].sh_type == SHT_REL)
> >  			err = apply_relocate(info->sechdrs, info->strtab,
> >  					     info->index.sym, i, mod);
> >  		else if (info->sechdrs[i].sh_type == SHT_RELA)
> 
> ... apply_relocations() is also iterating over the section headers (the
> diff context doesn't show it here, but i is an incrementing index over
> sechdrs[]).
> 
> So if there is more than one KLP relocation section, we'll process them
> multiple times.  At least the x86 relocation code will detect this and
> fail the module load with an invalid relocation (existing value not
> zero).

Ah, yes, good catch!

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 18:25     ` Josh Poimboeuf
@ 2020-04-20 19:01       ` Joe Lawrence
  2020-04-20 19:11         ` Josh Poimboeuf
  0 siblings, 1 reply; 38+ messages in thread
From: Joe Lawrence @ 2020-04-20 19:01 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, Apr 20, 2020 at 01:25:16PM -0500, Josh Poimboeuf wrote:
> On Mon, Apr 20, 2020 at 01:57:51PM -0400, Joe Lawrence wrote:
> > On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> > > 
> > > [ ... snip ... ]
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 40cfac8156fd..5fda3afc0285 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > 
> > > [ ... snip ... ]
> > > 
> > > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > > +			  const char *shstrtab, const char *strtab,
> > > +			  unsigned int symndx, struct module *pmod,
> > > +			  const char *objname)
> > >  {
> > >  	int i, cnt, ret = 0;
> > > -	const char *objname, *secname;
> > >  	char sec_objname[MODULE_NAME_LEN];
> > >  	Elf_Shdr *sec;
> > >  
> > > -	if (WARN_ON(!klp_is_object_loaded(obj)))
> > > -		return -EINVAL;
> > > -
> > > -	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > > -
> > >  	/* For each klp relocation section */
> > > -	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > > -		sec = pmod->klp_info->sechdrs + i;
> > > -		secname = pmod->klp_info->secstrings + sec->sh_name;
> > > +	for (i = 1; i < ehdr->e_shnum; i++) {
> > > +		sec = sechdrs + i;
> > 
> > Hi Josh, minor bug:
> > 
> > Note the for loop through the section headers in
> > klp_write_relocations(), but its calling function ...
> > 
> > > [ ... snip ... ]
> > > 
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 646f1e2330d2..d36ea8a8c3ec 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> > >  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> > >  			continue;
> > >  
> > > -		/* Livepatch relocation sections are applied by livepatch */
> > >  		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > > -			continue;
> > > -
> > > -		if (info->sechdrs[i].sh_type == SHT_REL)
> > > +			err = klp_write_relocations(info->hdr, info->sechdrs,
> > > +						    info->secstrings,
> > > +						    info->strtab,
> > > +						    info->index.sym, mod, NULL);
> > > +		else if (info->sechdrs[i].sh_type == SHT_REL)
> > >  			err = apply_relocate(info->sechdrs, info->strtab,
> > >  					     info->index.sym, i, mod);
> > >  		else if (info->sechdrs[i].sh_type == SHT_RELA)
> > 
> > ... apply_relocations() is also iterating over the section headers (the
> > diff context doesn't show it here, but i is an incrementing index over
> > sechdrs[]).
> > 
> > So if there is more than one KLP relocation section, we'll process them
> > multiple times.  At least the x86 relocation code will detect this and
> > fail the module load with an invalid relocation (existing value not
> > zero).
> 
> Ah, yes, good catch!
> 

The same test case passed with a small modification to push the foreach
KLP section part to a kernel/livepatch/core.c local function and
exposing the klp_resolve_symbols() + apply_relocate_add() for a given
section to kernel/module.c.  Something like following...

-- Joe

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 8b6886c2d5c7..516f285ccc82 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -232,9 +232,9 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
 struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
 struct klp_state *klp_get_prev_state(unsigned long id);
 
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
-			  const char *shstrtab, const char *strtab,
-			  unsigned int symindex, struct module *pmod,
+int klp_write_relocations(Elf_Shdr *sechdrs, const char *shstrtab,
+			  const char *strtab, unsigned int symndx,
+			  unsigned int relsec, struct module *pmod,
 			  const char *objname);
 
 /* Used to annotate symbol relocations in livepatches */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d2610f63e70b..d74fd7d10f16 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -285,14 +285,48 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
  *    the to-be-patched module to be loaded and patched sometime *after* the
  *    klp module is loaded.
  */
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
-			  const char *shstrtab, const char *strtab,
-			  unsigned int symndx, struct module *pmod,
+
+int klp_write_relocations(Elf_Shdr *sechdrs, const char *shstrtab,
+			  const char *strtab, unsigned int symndx,
+			  unsigned int relsec, struct module *pmod,
 			  const char *objname)
 {
-	int i, cnt, ret = 0;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
+	int cnt, ret;
+
+	sec = sechdrs + relsec;
+
+	/*
+	 * Format: .klp.rela.sec_objname.section_name
+	 * See comment in klp_resolve_symbols() for an explanation
+	 * of the selected field width value.
+	 */
+	cnt = sscanf(shstrtab + sec->sh_name, KLP_RELA_PREFIX "%55[^.]",
+		     sec_objname);
+	if (cnt != 1) {
+		pr_err("section %s has an incorrectly formatted name\n",
+		       shstrtab + sec->sh_name);
+		return -EINVAL;
+	}
+
+	if (strcmp(objname ? objname : "vmlinux", sec_objname))
+		return 0;
+
+	ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
+	if (ret)
+		return ret;
+
+	return apply_relocate_add(sechdrs, strtab, symndx, relsec, pmod);
+}
+
+static int klp_write_all_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+				     const char *shstrtab, const char *strtab,
+				     unsigned int symndx, struct module *pmod,
+				     const char *objname)
+{
+	int i, ret;
+	Elf_Shdr *sec;
 
 	/* For each klp relocation section */
 	for (i = 1; i < ehdr->e_shnum; i++) {
@@ -300,34 +334,13 @@ int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
 			continue;
 
-		/*
-		 * Format: .klp.rela.sec_objname.section_name
-		 * See comment in klp_resolve_symbols() for an explanation
-		 * of the selected field width value.
-		 */
-		cnt = sscanf(shstrtab + sec->sh_name, KLP_RELA_PREFIX "%55[^.]",
-			     sec_objname);
-		if (cnt != 1) {
-			pr_err("section %s has an incorrectly formatted name\n",
-			       shstrtab + sec->sh_name);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (strcmp(objname ? objname : "vmlinux", sec_objname))
-			continue;
-
-		ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
-					  sec_objname);
+		ret = klp_write_relocations(sechdrs, shstrtab, strtab, symndx,
+					    i, pmod, objname);
 		if (ret)
-			break;
-
-		ret = apply_relocate_add(sechdrs, strtab, symndx, i, pmod);
-		if (ret)
-			break;
+			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 /*
@@ -773,11 +786,11 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 		 * written earlier during the initialization of the klp module
 		 * itself.
 		 */
-		ret = klp_write_relocations(&info->hdr, info->sechdrs,
-					    info->secstrings,
-					    patch->mod->core_kallsyms.strtab,
-					    info->symndx, patch->mod,
-					    obj->name);
+		ret = klp_write_all_relocations(&info->hdr, info->sechdrs,
+						info->secstrings,
+						patch->mod->core_kallsyms.strtab,
+						info->symndx, patch->mod,
+						obj->name);
 		if (ret)
 			return ret;
 	}
diff --git a/kernel/module.c b/kernel/module.c
index 86736e2ff73d..04e5f5d55eb4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2322,10 +2322,11 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 			continue;
 
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			err = klp_write_relocations(info->hdr, info->sechdrs,
+			err = klp_write_relocations(info->sechdrs,
 						    info->secstrings,
 						    info->strtab,
-						    info->index.sym, mod, NULL);
+						    info->index.sym, i, mod,
+						    NULL);
 		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 19:01       ` Joe Lawrence
@ 2020-04-20 19:11         ` Josh Poimboeuf
  2020-04-20 19:49           ` Joe Lawrence
  2020-04-23  1:10           ` Joe Lawrence
  0 siblings, 2 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-20 19:11 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > ... apply_relocations() is also iterating over the section headers (the
> > > diff context doesn't show it here, but i is an incrementing index over
> > > sechdrs[]).
> > > 
> > > So if there is more than one KLP relocation section, we'll process them
> > > multiple times.  At least the x86 relocation code will detect this and
> > > fail the module load with an invalid relocation (existing value not
> > > zero).
> > 
> > Ah, yes, good catch!
> > 
> 
> The same test case passed with a small modification to push the foreach
> KLP section part to a kernel/livepatch/core.c local function and
> exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> section to kernel/module.c.  Something like following...

I came up with something very similar, though I named them
klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
argument order a bit (module first).  Since it sounds like you have a
test, could you try this one?

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 533359e48c39..fb1a3de39726 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -231,10 +231,10 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
 struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
 struct klp_state *klp_get_prev_state(unsigned long id);
 
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
-			  const char *shstrtab, const char *strtab,
-			  unsigned int symindex, struct module *pmod,
-			  const char *objname);
+int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+			     const char *shstrtab, const char *strtab,
+			     unsigned int symindex, unsigned int secindex,
+			     const char *objname);
 
 #else /* !CONFIG_LIVEPATCH */
 
@@ -245,10 +245,10 @@ static inline void klp_update_patch_state(struct task_struct *task) {}
 static inline void klp_copy_process(struct task_struct *child) {}
 
 static inline
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
-			  const char *shstrtab, const char *strtab,
-			  unsigned int symindex, struct module *pmod,
-			  const char *objname)
+int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+			     const char *shstrtab, const char *strtab,
+			     unsigned int symindex, unsigned int secindex,
+			     const char *objname);
 {
 	return 0;
 }
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3ff886b911ae..89c5cb962c54 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -285,49 +285,37 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
  *    the to-be-patched module to be loaded and patched sometime *after* the
  *    klp module is loaded.
  */
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
-			  const char *shstrtab, const char *strtab,
-			  unsigned int symndx, struct module *pmod,
-			  const char *objname)
+int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+			     const char *shstrtab, const char *strtab,
+			     unsigned int symndx, unsigned int secndx,
+			     const char *objname)
 {
-	int i, cnt, ret = 0;
+	int cnt, ret;
 	char sec_objname[MODULE_NAME_LEN];
-	Elf_Shdr *sec;
+	Elf_Shdr *sec = sechdrs + secndx;
 
-	/* For each klp relocation section */
-	for (i = 1; i < ehdr->e_shnum; i++) {
-		sec = sechdrs + i;
-		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
-			continue;
-
-		/*
-		 * Format: .klp.rela.sec_objname.section_name
-		 * See comment in klp_resolve_symbols() for an explanation
-		 * of the selected field width value.
-		 */
-		cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
-			     sec_objname);
-		if (cnt != 1) {
-			pr_err("section %s has an incorrectly formatted name\n",
-			       shstrtab + sec->sh_name);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (strcmp(objname ? objname : "vmlinux", sec_objname))
-			continue;
+	/*
+	 * Format: .klp.rela.sec_objname.section_name
+	 * See comment in klp_resolve_symbols() for an explanation
+	 * of the selected field width value.
+	 */
+	cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
+		     sec_objname);
+	if (cnt != 1) {
+		pr_err("section %s has an incorrectly formatted name\n",
+		       shstrtab + sec->sh_name);
+		return -EINVAL;
+	}
 
-		ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
-					  sec_objname);
-		if (ret)
-			break;
+	if (strcmp(objname ? objname : "vmlinux", sec_objname))
+		return 0;
 
-		ret = apply_relocate_add(sechdrs, strtab, symndx, i, pmod);
-		if (ret)
-			break;
-	}
+	ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
+				  sec_objname);
+	if (ret)
+		return ret;
 
-	return ret;
+	return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
 }
 
 /*
@@ -758,13 +746,34 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 			   func->old_sympos ? func->old_sympos : 1);
 }
 
+int klp_apply_object_relocs(struct klp_patch *patch, struct klp_object *obj)
+{
+	int i, ret;
+	struct klp_modinfo *info = patch->mod->klp_info;
+
+	for (i = 1; i < info->hdr.e_shnum; i++) {
+		Elf_Shdr *sec = info->sechdrs + i;
+
+		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
+			continue;
+
+		ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
+					       info->secstrings,
+					       patch->mod->core_kallsyms.strtab,
+					       info->symndx, i, obj->name);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
 {
 	struct klp_func *func;
 	int ret;
-	struct klp_modinfo *info = patch->mod->klp_info;
 
 	if (klp_is_module(obj)) {
 		/*
@@ -773,11 +782,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 		 * written earlier during the initialization of the klp module
 		 * itself.
 		 */
-		ret = klp_write_relocations(&info->hdr, info->sechdrs,
-					    info->secstrings,
-					    patch->mod->core_kallsyms.strtab,
-					    info->symndx, patch->mod,
-					    obj->name);
+		ret = klp_apply_object_relocs(patch, obj);
 		if (ret)
 			return ret;
 	}
diff --git a/kernel/module.c b/kernel/module.c
index b1d30ad67e82..3ba024afe379 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2322,10 +2322,11 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 			continue;
 
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			err = klp_write_relocations(info->hdr, info->sechdrs,
-						    info->secstrings,
-						    info->strtab,
-						    info->index.sym, mod, NULL);
+			err = klp_apply_section_relocs(mod, info->sechdrs,
+						       info->secstrings,
+						       info->strtab,
+						       info->index.sym, i,
+						       NULL);
 		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 19:11         ` Josh Poimboeuf
@ 2020-04-20 19:49           ` Joe Lawrence
  2020-04-20 19:51             ` Josh Poimboeuf
  2020-04-23  1:10           ` Joe Lawrence
  1 sibling, 1 reply; 38+ messages in thread
From: Joe Lawrence @ 2020-04-20 19:49 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, Apr 20, 2020 at 02:11:17PM -0500, Josh Poimboeuf wrote:
> On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > > ... apply_relocations() is also iterating over the section headers (the
> > > > diff context doesn't show it here, but i is an incrementing index over
> > > > sechdrs[]).
> > > > 
> > > > So if there is more than one KLP relocation section, we'll process them
> > > > multiple times.  At least the x86 relocation code will detect this and
> > > > fail the module load with an invalid relocation (existing value not
> > > > zero).
> > > 
> > > Ah, yes, good catch!
> > > 
> > 
> > The same test case passed with a small modification to push the foreach
> > KLP section part to a kernel/livepatch/core.c local function and
> > exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> > section to kernel/module.c.  Something like following...
> 
> I came up with something very similar, though I named them
> klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
> argument order a bit (module first).  Since it sounds like you have a
> test, could you try this one?
> 

LGTM.  I have a few klp-convert selftests that I've been slowly
tinkering on and they all load/run successfully with this version. :)

-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 19:49           ` Joe Lawrence
@ 2020-04-20 19:51             ` Josh Poimboeuf
  0 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-20 19:51 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, Apr 20, 2020 at 03:49:00PM -0400, Joe Lawrence wrote:
> On Mon, Apr 20, 2020 at 02:11:17PM -0500, Josh Poimboeuf wrote:
> > On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > > > ... apply_relocations() is also iterating over the section headers (the
> > > > > diff context doesn't show it here, but i is an incrementing index over
> > > > > sechdrs[]).
> > > > > 
> > > > > So if there is more than one KLP relocation section, we'll process them
> > > > > multiple times.  At least the x86 relocation code will detect this and
> > > > > fail the module load with an invalid relocation (existing value not
> > > > > zero).
> > > > 
> > > > Ah, yes, good catch!
> > > > 
> > > 
> > > The same test case passed with a small modification to push the foreach
> > > KLP section part to a kernel/livepatch/core.c local function and
> > > exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> > > section to kernel/module.c.  Something like following...
> > 
> > I came up with something very similar, though I named them
> > klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
> > argument order a bit (module first).  Since it sounds like you have a
> > test, could you try this one?
> > 
> 
> LGTM.  I have a few klp-convert selftests that I've been slowly
> tinkering on and they all load/run successfully with this version. :)

Good to hear, thanks!  Hooray selftests :-)

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 17:57   ` Joe Lawrence
  2020-04-20 18:25     ` Josh Poimboeuf
@ 2020-04-21 11:54     ` Miroslav Benes
  1 sibling, 0 replies; 38+ messages in thread
From: Miroslav Benes @ 2020-04-21 11:54 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Josh Poimboeuf, live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, 20 Apr 2020, Joe Lawrence wrote:

> On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> > 
> > [ ... snip ... ]
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 40cfac8156fd..5fda3afc0285 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > 
> > [ ... snip ... ]
> > 
> > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > +			  const char *shstrtab, const char *strtab,
> > +			  unsigned int symndx, struct module *pmod,
> > +			  const char *objname)
> >  {
> >  	int i, cnt, ret = 0;
> > -	const char *objname, *secname;
> >  	char sec_objname[MODULE_NAME_LEN];
> >  	Elf_Shdr *sec;
> >  
> > -	if (WARN_ON(!klp_is_object_loaded(obj)))
> > -		return -EINVAL;
> > -
> > -	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > -
> >  	/* For each klp relocation section */
> > -	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > -		sec = pmod->klp_info->sechdrs + i;
> > -		secname = pmod->klp_info->secstrings + sec->sh_name;
> > +	for (i = 1; i < ehdr->e_shnum; i++) {
> > +		sec = sechdrs + i;
> 
> Hi Josh, minor bug:
> 
> Note the for loop through the section headers in
> klp_write_relocations(), but its calling function ...
> 
> > [ ... snip ... ]
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 646f1e2330d2..d36ea8a8c3ec 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> >  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> >  			continue;
> >  
> > -		/* Livepatch relocation sections are applied by livepatch */
> >  		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > -			continue;
> > -
> > -		if (info->sechdrs[i].sh_type == SHT_REL)
> > +			err = klp_write_relocations(info->hdr, info->sechdrs,
> > +						    info->secstrings,
> > +						    info->strtab,
> > +						    info->index.sym, mod, NULL);
> > +		else if (info->sechdrs[i].sh_type == SHT_REL)
> >  			err = apply_relocate(info->sechdrs, info->strtab,
> >  					     info->index.sym, i, mod);
> >  		else if (info->sechdrs[i].sh_type == SHT_RELA)
> 
> ... apply_relocations() is also iterating over the section headers (the
> diff context doesn't show it here, but i is an incrementing index over
> sechdrs[]).
> 
> So if there is more than one KLP relocation section, we'll process them
> multiple times.  At least the x86 relocation code will detect this and
> fail the module load with an invalid relocation (existing value not
> zero).

The last paragraph confused me a little. I'm sending the following, so it 
is archived publicly.

If there is more than one KLP relocation section in a patch module, let's 
say for vmlinux and some arbitrary module, klp_write_relocations() will 
be called multiple times from apply_relocations(). Each time with NULL as 
the last parameter, so each time vmlinux relocation section will be 
processed and x86 relocation code will detect this the second time and 
fail.

If there was no relocation section for vmlinux, but for multiple arbitrary 
modules, all should be "fine", because klp_write_relocations() would just 
skip everything.

Anyway, good catch, I missed it completely.

Miroslav

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-17 14:04 ` [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations Josh Poimboeuf
@ 2020-04-22 12:28   ` Miroslav Benes
  2020-04-24 13:43     ` Josh Poimboeuf
  2020-04-22 14:40   ` Gerald Schaefer
  1 sibling, 1 reply; 38+ messages in thread
From: Miroslav Benes @ 2020-04-22 12:28 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu,
	linux-s390, heiko.carstens

> +int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> +		       unsigned int symindex, unsigned int relsec,
> +		       struct module *me)
> +{
> +	int ret;
> +	bool early = me->state == MODULE_STATE_UNFORMED;
> +	void *(*write)(void *, const void *, size_t) = memcpy;
> +
> +	if (!early) {
> +		write = s390_kernel_write;
> +		mutex_lock(&text_mutex);
> +	}
> +
> +	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> +				   write);
> +
> +	if (!early)
> +		mutex_unlock(&text_mutex);
> +
> +	return ret;
> +}

It means that you can take text_mutex the second time here because it 
is still taken in klp_init_object_loaded(). It is removed later in the 
series though. The same applies for x86 patch.

Also, s390_kernel_write() grabs s390_kernel_write_lock spinlock before 
writing anything, so maybe text_mutex is not really needed as long as 
s390_kernel_write is called everywhere for text patching.

Miroslav

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-17 14:04 ` [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations Josh Poimboeuf
  2020-04-22 12:28   ` Miroslav Benes
@ 2020-04-22 14:40   ` Gerald Schaefer
  2020-04-22 15:21     ` Gerald Schaefer
  1 sibling, 1 reply; 38+ messages in thread
From: Gerald Schaefer @ 2020-04-22 14:40 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu,
	linux-s390, heiko.carstens, Gerald Schaefer, Vasily Gorbik

On Fri, 17 Apr 2020 09:04:31 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> From: Peter Zijlstra <peterz@infradead.org>
> 
> Because of late module patching, a livepatch module needs to be able to
> apply some of its relocations well after it has been loaded.  Instead of
> playing games with module_{dis,en}able_ro(), use existing text poking
> mechanisms to apply relocations after module loading.
> 
> So far only x86, s390 and Power have HAVE_LIVEPATCH but only the first
> two also have STRICT_MODULE_RWX.
> 
> This will allow removal of the last module_disable_ro() usage in
> livepatch.  The ultimate goal is to completely disallow making
> executable mappings writable.
> 
> Also, for the late patching case, use text_mutex, which is supposed to
> be held for all runtime text patching operations.
> 
> [ jpoimboe: Split up patches.  Use mod state to determine whether
> 	    memcpy() can be used.  Add text_mutex.  Make it build. ]
> 
> Cc: linux-s390@vger.kernel.org
> Cc: heiko.carstens@de.ibm.com
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/s390/kernel/module.c | 125 ++++++++++++++++++++++++--------------
>  1 file changed, 79 insertions(+), 46 deletions(-)

Sorry, just noticed this. Heiko will return next month, and I'm not
really familiar with s390 livepatching. Adding Vasily, he might
have some more insight.

So, I might be completely wrong here, but using s390_kernel_write()
for writing to anything other than 1:1 mapped kernel, should go
horribly wrong, as that runs w/o DAT. It would allow to bypass
DAT write protection, which I assume is why you want to use it,
but it should not work on module text section, as that would be
in vmalloc space and not 1:1 mapped kernel memory.

Not quite sure how to test / trigger this, did this really work for
you on s390?

Regards,
Gerald


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-22 14:40   ` Gerald Schaefer
@ 2020-04-22 15:21     ` Gerald Schaefer
  2020-04-22 19:46       ` Josh Poimboeuf
  0 siblings, 1 reply; 38+ messages in thread
From: Gerald Schaefer @ 2020-04-22 15:21 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu,
	linux-s390, heiko.carstens, Vasily Gorbik, Gerald Schaefer

On Wed, 22 Apr 2020 16:40:37 +0200
Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:

> On Fri, 17 Apr 2020 09:04:31 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > From: Peter Zijlstra <peterz@infradead.org>
> > 
> > Because of late module patching, a livepatch module needs to be able to
> > apply some of its relocations well after it has been loaded.  Instead of
> > playing games with module_{dis,en}able_ro(), use existing text poking
> > mechanisms to apply relocations after module loading.
> > 
> > So far only x86, s390 and Power have HAVE_LIVEPATCH but only the first
> > two also have STRICT_MODULE_RWX.
> > 
> > This will allow removal of the last module_disable_ro() usage in
> > livepatch.  The ultimate goal is to completely disallow making
> > executable mappings writable.
> > 
> > Also, for the late patching case, use text_mutex, which is supposed to
> > be held for all runtime text patching operations.
> > 
> > [ jpoimboe: Split up patches.  Use mod state to determine whether
> > 	    memcpy() can be used.  Add text_mutex.  Make it build. ]
> > 
> > Cc: linux-s390@vger.kernel.org
> > Cc: heiko.carstens@de.ibm.com
> > Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/s390/kernel/module.c | 125 ++++++++++++++++++++++++--------------
> >  1 file changed, 79 insertions(+), 46 deletions(-)
> 
> Sorry, just noticed this. Heiko will return next month, and I'm not
> really familiar with s390 livepatching. Adding Vasily, he might
> have some more insight.
> 
> So, I might be completely wrong here, but using s390_kernel_write()
> for writing to anything other than 1:1 mapped kernel, should go
> horribly wrong, as that runs w/o DAT. It would allow to bypass
> DAT write protection, which I assume is why you want to use it,
> but it should not work on module text section, as that would be
> in vmalloc space and not 1:1 mapped kernel memory.
> 
> Not quite sure how to test / trigger this, did this really work for
> you on s390?

OK, using s390_kernel_write() as default write function for module
relocation seems to work fine for me, so apparently I am missing /
mixing up something. Sorry for the noise, please ignore my concern.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-22 15:21     ` Gerald Schaefer
@ 2020-04-22 19:46       ` Josh Poimboeuf
  2020-04-23 12:33         ` Gerald Schaefer
  0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-22 19:46 UTC (permalink / raw)
  To: Gerald Schaefer
  Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu,
	linux-s390, heiko.carstens, Vasily Gorbik, Joe Lawrence

On Wed, Apr 22, 2020 at 05:21:26PM +0200, Gerald Schaefer wrote:
> > Sorry, just noticed this. Heiko will return next month, and I'm not
> > really familiar with s390 livepatching. Adding Vasily, he might
> > have some more insight.
> > 
> > So, I might be completely wrong here, but using s390_kernel_write()
> > for writing to anything other than 1:1 mapped kernel, should go
> > horribly wrong, as that runs w/o DAT. It would allow to bypass
> > DAT write protection, which I assume is why you want to use it,
> > but it should not work on module text section, as that would be
> > in vmalloc space and not 1:1 mapped kernel memory.
> > 
> > Not quite sure how to test / trigger this, did this really work for
> > you on s390?
> 
> OK, using s390_kernel_write() as default write function for module
> relocation seems to work fine for me, so apparently I am missing /
> mixing up something. Sorry for the noise, please ignore my concern.

Hi Gerald,

I think you were right.  Joe found the below panic with his klp-convert
tests.

Your test was probably the early module loading case (normal relocations
before write protection), rather than the late case.  Not sure why that
would work, but calling s390_kernel_write() late definitely seems to be
broken.

Is there some other way to write vmalloc'ed s390 text without using
module_disable_ro()?

[   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
[   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
[   50.294480] Fault in home space mode while using kernel ASCE.
[   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
[   50.294557] Oops: 0004 ilc:3 [#1] SMP
[   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
[   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
[   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
[   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
[   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
[   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
[   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
[   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
[   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
[   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
[   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
[   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
[   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
[   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
[   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
[   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
[   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
[   50.295369] Call Trace:
[   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
[   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
[   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
[   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
[   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
[   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
[   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]
[   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
[   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
[   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
[   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
[   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
[   50.295416] Last Breaking-Event-Address:
[   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
[   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early
  2020-04-20 19:11         ` Josh Poimboeuf
  2020-04-20 19:49           ` Joe Lawrence
@ 2020-04-23  1:10           ` Joe Lawrence
  1 sibling, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-23  1:10 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu

On Mon, Apr 20, 2020 at 02:11:17PM -0500, Josh Poimboeuf wrote:
> On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > > ... apply_relocations() is also iterating over the section headers (the
> > > > diff context doesn't show it here, but i is an incrementing index over
> > > > sechdrs[]).
> > > > 
> > > > So if there is more than one KLP relocation section, we'll process them
> > > > multiple times.  At least the x86 relocation code will detect this and
> > > > fail the module load with an invalid relocation (existing value not
> > > > zero).
> > > 
> > > Ah, yes, good catch!
> > > 
> > 
> > The same test case passed with a small modification to push the foreach
> > KLP section part to a kernel/livepatch/core.c local function and
> > exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> > section to kernel/module.c.  Something like following...
> 
> I came up with something very similar, though I named them
> klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
> argument order a bit (module first).  Since it sounds like you have a
> test, could you try this one?
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 533359e48c39..fb1a3de39726 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> 
> [ ... snip ... ]
> 
> @@ -245,10 +245,10 @@ static inline void klp_update_patch_state(struct task_struct *task) {}
>  static inline void klp_copy_process(struct task_struct *child) {}
>  
>  static inline
> -int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> -			  const char *shstrtab, const char *strtab,
> -			  unsigned int symindex, struct module *pmod,
> -			  const char *objname)
> +int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> +			     const char *shstrtab, const char *strtab,
> +			     unsigned int symindex, unsigned int secindex,
> +			     const char *objname);
                                                ^^
Whoops, stray semicolon in !CONFIG_LIVEPATCH case.  I found it by
botching my cross-compiling .config, but the build-bot might find it
when you push your branch.

>  {
>  	return 0;
>  }

-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-22 19:46       ` Josh Poimboeuf
@ 2020-04-23 12:33         ` Gerald Schaefer
  2020-04-23 13:22           ` Miroslav Benes
  0 siblings, 1 reply; 38+ messages in thread
From: Gerald Schaefer @ 2020-04-23 12:33 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu,
	linux-s390, heiko.carstens, Vasily Gorbik, Joe Lawrence,
	Gerald Schaefer

On Wed, 22 Apr 2020 14:46:05 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Apr 22, 2020 at 05:21:26PM +0200, Gerald Schaefer wrote:
> > > Sorry, just noticed this. Heiko will return next month, and I'm not
> > > really familiar with s390 livepatching. Adding Vasily, he might
> > > have some more insight.
> > > 
> > > So, I might be completely wrong here, but using s390_kernel_write()
> > > for writing to anything other than 1:1 mapped kernel, should go
> > > horribly wrong, as that runs w/o DAT. It would allow to bypass
> > > DAT write protection, which I assume is why you want to use it,
> > > but it should not work on module text section, as that would be
> > > in vmalloc space and not 1:1 mapped kernel memory.
> > > 
> > > Not quite sure how to test / trigger this, did this really work for
> > > you on s390?
> > 
> > OK, using s390_kernel_write() as default write function for module
> > relocation seems to work fine for me, so apparently I am missing /
> > mixing up something. Sorry for the noise, please ignore my concern.
> 
> Hi Gerald,
> 
> I think you were right.  Joe found the below panic with his klp-convert
> tests.
> 
> Your test was probably the early module loading case (normal relocations
> before write protection), rather than the late case.  Not sure why that
> would work, but calling s390_kernel_write() late definitely seems to be
> broken.
> 
> Is there some other way to write vmalloc'ed s390 text without using
> module_disable_ro()?
> 
> [   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
> [   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
> [   50.294480] Fault in home space mode while using kernel ASCE.
> [   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
> [   50.294557] Oops: 0004 ilc:3 [#1] SMP
> [   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
> wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
> [   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
> [   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
> [   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
> [   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
> [   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
> [   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
> [   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
> [   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
> [   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
> [   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
> [   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
> [   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
> [   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
> [   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
> [   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
> [   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
> 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
> 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
> [   50.295369] Call Trace:
> [   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
> [   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
> [   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
> [   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
> [   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
> [   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
> [   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]
> [   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
> [   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
> [   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
> [   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
> [   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
> [   50.295416] Last Breaking-Event-Address:
> [   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
> [   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops
> 

Hi Josh,

this is strange. While I would have expected an exception similar to
this, it really should have happened on the "sturg" instruction which
does the DAT-off store in s390_kernel_write(), and certainly not with
an ID of 0004 (protection). However, in your case, it happens on a
normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do
_not_ use the s390_kernel_write() function for RO module text patching,
but rather normal memory access. So I am pretty sure that this is not
related to the s390_kernel_write(), but some other issue, maybe some
place left where you still use normal memory access?

There is also some good news. While thinking about how to use "sturg"
for vmalloc addresses, I came up with the idea to use "lra" (load
real address) before that. Then I found out that we already do exactly
that in the inline assembly, so all should be fine. Well, maybe the
comment for s390_kernel_write() could be improved...

Vasily also found out that we apparently already use s390_kernel_write()
for module text, for alternatives, so I guess we can safely assume that
it should work fine in principle.

Regards,
Gerald


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 12:33         ` Gerald Schaefer
@ 2020-04-23 13:22           ` Miroslav Benes
  2020-04-23 14:12             ` Josh Poimboeuf
  2020-04-23 14:21             ` Joe Lawrence
  0 siblings, 2 replies; 38+ messages in thread
From: Miroslav Benes @ 2020-04-23 13:22 UTC (permalink / raw)
  To: Gerald Schaefer
  Cc: Josh Poimboeuf, live-patching, linux-kernel, Peter Zijlstra,
	Jessica Yu, linux-s390, heiko.carstens, Vasily Gorbik,
	Joe Lawrence

On Thu, 23 Apr 2020, Gerald Schaefer wrote:

> On Wed, 22 Apr 2020 14:46:05 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Wed, Apr 22, 2020 at 05:21:26PM +0200, Gerald Schaefer wrote:
> > > > Sorry, just noticed this. Heiko will return next month, and I'm not
> > > > really familiar with s390 livepatching. Adding Vasily, he might
> > > > have some more insight.
> > > > 
> > > > So, I might be completely wrong here, but using s390_kernel_write()
> > > > for writing to anything other than 1:1 mapped kernel, should go
> > > > horribly wrong, as that runs w/o DAT. It would allow to bypass
> > > > DAT write protection, which I assume is why you want to use it,
> > > > but it should not work on module text section, as that would be
> > > > in vmalloc space and not 1:1 mapped kernel memory.
> > > > 
> > > > Not quite sure how to test / trigger this, did this really work for
> > > > you on s390?
> > > 
> > > OK, using s390_kernel_write() as default write function for module
> > > relocation seems to work fine for me, so apparently I am missing /
> > > mixing up something. Sorry for the noise, please ignore my concern.
> > 
> > Hi Gerald,
> > 
> > I think you were right.  Joe found the below panic with his klp-convert
> > tests.
> > 
> > Your test was probably the early module loading case (normal relocations
> > before write protection), rather than the late case.  Not sure why that
> > would work, but calling s390_kernel_write() late definitely seems to be
> > broken.
> > 
> > Is there some other way to write vmalloc'ed s390 text without using
> > module_disable_ro()?
> > 
> > [   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
> > [   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
> > [   50.294480] Fault in home space mode while using kernel ASCE.
> > [   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
> > [   50.294557] Oops: 0004 ilc:3 [#1] SMP
> > [   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
> > wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
> > [   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
> > [   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
> > [   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
> > [   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
> > [   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
> > [   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
> > [   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
> > [   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
> > [   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
> > [   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
> > [   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
> > [   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
> > [   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
> > [   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
> > [   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
> > [   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
> > 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
> > 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
> > [   50.295369] Call Trace:
> > [   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
> > [   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
> > [   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
> > [   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
> > [   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
> > [   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
> > [   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]
> > [   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
> > [   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
> > [   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
> > [   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
> > [   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
> > [   50.295416] Last Breaking-Event-Address:
> > [   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
> > [   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops
> > 
> 
> Hi Josh,
> 
> this is strange. While I would have expected an exception similar to
> this, it really should have happened on the "sturg" instruction which
> does the DAT-off store in s390_kernel_write(), and certainly not with
> an ID of 0004 (protection). However, in your case, it happens on a
> normal store instruction, with 0004 indicating a protection exception.
> 
> This is more like what I would expect e.g. in the case where you do
> _not_ use the s390_kernel_write() function for RO module text patching,
> but rather normal memory access. So I am pretty sure that this is not
> related to the s390_kernel_write(), but some other issue, maybe some
> place left where you still use normal memory access?

The call trace above also suggests that it is not a late relocation, no? 
The path is from KLP module init function through klp_enable_patch. It should 
mean that the to-be-patched object is loaded (it must be a module thanks 
to a check klp_init_object_loaded(), vmlinux relocations were processed 
earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() 
should be used. What are we missing?

Joe, could you debug this a bit, please?
 
> There is also some good news. While thinking about how to use "sturg"
> for vmalloc addresses, I came up with the idea to use "lra" (load
> real address) before that. Then I found out that we already do exactly
> that in the inline assembly, so all should be fine. Well, maybe the
> comment for s390_kernel_write() could be improved...
> 
> Vasily also found out that we apparently already use s390_kernel_write()
> for module text, for alternatives, so I guess we can safely assume that
> it should work fine in principle.

Phew, okay. I noticed that s390_kernel_write() is called on a couple of 
places already (kprobes) and I wondered where the trick was.

Thanks
Miroslav

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 13:22           ` Miroslav Benes
@ 2020-04-23 14:12             ` Josh Poimboeuf
  2020-04-23 18:10               ` Josh Poimboeuf
  2020-04-23 14:21             ` Joe Lawrence
  1 sibling, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-23 14:12 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Gerald Schaefer, live-patching, linux-kernel, Peter Zijlstra,
	Jessica Yu, linux-s390, heiko.carstens, Vasily Gorbik,
	Joe Lawrence

On Thu, Apr 23, 2020 at 03:22:06PM +0200, Miroslav Benes wrote:
> > > [   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
> > > [   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
> > > [   50.294480] Fault in home space mode while using kernel ASCE.
> > > [   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
> > > [   50.294557] Oops: 0004 ilc:3 [#1] SMP
> > > [   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
> > > wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
> > > [   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
> > > [   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
> > > [   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
> > > [   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
> > > [   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
> > > [   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
> > > [   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
> > > [   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
> > > [   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
> > > [   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
> > > [   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
> > > [   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
> > > [   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
> > > [   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
> > > [   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
> > > [   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
> > > 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
> > > 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
> > > [   50.295369] Call Trace:
> > > [   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
> > > [   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
> > > [   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
> > > [   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
> > > [   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
> > > [   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
> > > [   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]
> > > [   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
> > > [   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
> > > [   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
> > > [   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
> > > [   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
> > > [   50.295416] Last Breaking-Event-Address:
> > > [   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
> > > [   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops
> >
> > this is strange. While I would have expected an exception similar to
> > this, it really should have happened on the "sturg" instruction which
> > does the DAT-off store in s390_kernel_write(), and certainly not with
> > an ID of 0004 (protection). However, in your case, it happens on a
> > normal store instruction, with 0004 indicating a protection exception.
> > 
> > This is more like what I would expect e.g. in the case where you do
> > _not_ use the s390_kernel_write() function for RO module text patching,
> > but rather normal memory access. So I am pretty sure that this is not
> > related to the s390_kernel_write(), but some other issue, maybe some
> > place left where you still use normal memory access?
> 
> The call trace above also suggests that it is not a late relocation, no? 
> The path is from KLP module init function through klp_enable_patch. It should 
> mean that the to-be-patched object is loaded (it must be a module thanks 
> to a check klp_init_object_loaded(), vmlinux relocations were processed 
> earlier in apply_relocations()).
> 
> However, the KLP module state here must be COMING, so s390_kernel_write() 
> should be used. What are we missing?

I'm also scratching my head.  It _should_ be using s390_kernel_write()
based on the module state, but I don't see that on the stack trace.

This trace (and Gerald's comment) seem to imply it's using
__builtin_memcpy(), which might expected for UNFORMED state.

Weird...

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 13:22           ` Miroslav Benes
  2020-04-23 14:12             ` Josh Poimboeuf
@ 2020-04-23 14:21             ` Joe Lawrence
  1 sibling, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-23 14:21 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Gerald Schaefer, Josh Poimboeuf, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik

On Thu, Apr 23, 2020 at 03:22:06PM +0200, Miroslav Benes wrote:
> On Thu, 23 Apr 2020, Gerald Schaefer wrote:
> 
> > On Wed, 22 Apr 2020 14:46:05 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > On Wed, Apr 22, 2020 at 05:21:26PM +0200, Gerald Schaefer wrote:
> > > > > Sorry, just noticed this. Heiko will return next month, and I'm not
> > > > > really familiar with s390 livepatching. Adding Vasily, he might
> > > > > have some more insight.
> > > > > 
> > > > > So, I might be completely wrong here, but using s390_kernel_write()
> > > > > for writing to anything other than 1:1 mapped kernel, should go
> > > > > horribly wrong, as that runs w/o DAT. It would allow to bypass
> > > > > DAT write protection, which I assume is why you want to use it,
> > > > > but it should not work on module text section, as that would be
> > > > > in vmalloc space and not 1:1 mapped kernel memory.
> > > > > 
> > > > > Not quite sure how to test / trigger this, did this really work for
> > > > > you on s390?
> > > > 
> > > > OK, using s390_kernel_write() as default write function for module
> > > > relocation seems to work fine for me, so apparently I am missing /
> > > > mixing up something. Sorry for the noise, please ignore my concern.
> > > 
> > > Hi Gerald,
> > > 
> > > I think you were right.  Joe found the below panic with his klp-convert
> > > tests.
> > > 
> > > Your test was probably the early module loading case (normal relocations
> > > before write protection), rather than the late case.  Not sure why that
> > > would work, but calling s390_kernel_write() late definitely seems to be
> > > broken.
> > > 
> > > Is there some other way to write vmalloc'ed s390 text without using
> > > module_disable_ro()?
> > > 
> > > [   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
> > > [   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
> > > [   50.294480] Fault in home space mode while using kernel ASCE.
> > > [   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
> > > [   50.294557] Oops: 0004 ilc:3 [#1] SMP
> > > [   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
> > > wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
> > > [   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
> > > [   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
> > > [   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
> > > [   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
> > > [   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
> > > [   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
> > > [   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
> > > [   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
> > > [   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
> > > [   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
> > > [   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
> > > [   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
> > > [   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
> > > [   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
> > > [   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
> > > [   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
> > > 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
> > > 01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
> > > [   50.295369] Call Trace:
> > > [   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
> > > [   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
> > > [   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
> > > [   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
> > > [   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
> > > [   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
> > > [   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]
> > > [   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
> > > [   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
> > > [   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
> > > [   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
> > > [   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
> > > [   50.295416] Last Breaking-Event-Address:
> > > [   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
> > > [   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops
> > > 
> > 
> > Hi Josh,
> > 
> > this is strange. While I would have expected an exception similar to
> > this, it really should have happened on the "sturg" instruction which
> > does the DAT-off store in s390_kernel_write(), and certainly not with
> > an ID of 0004 (protection). However, in your case, it happens on a
> > normal store instruction, with 0004 indicating a protection exception.
> > 
> > This is more like what I would expect e.g. in the case where you do
> > _not_ use the s390_kernel_write() function for RO module text patching,
> > but rather normal memory access. So I am pretty sure that this is not
> > related to the s390_kernel_write(), but some other issue, maybe some
> > place left where you still use normal memory access?
> 
> The call trace above also suggests that it is not a late relocation, no? 

You are correct, details below...

> The path is from KLP module init function through klp_enable_patch. It should 
> mean that the to-be-patched object is loaded (it must be a module thanks 
> to a check klp_init_object_loaded(), vmlinux relocations were processed 
> earlier in apply_relocations()).
> 
> However, the KLP module state here must be COMING, so s390_kernel_write() 
> should be used. What are we missing?
> 
> Joe, could you debug this a bit, please?
>  

Here is the combined branch that I tested yesterday:
https://github.com/joe-lawrence/linux/tree/jp-v2-klp-convert

That combined WIP klp-convert changes merged on top of Josh's v2 patches
(and follow-ups) posted here.  Before I merged with Josh's changes, the
klp-convert code + tests ran without incident.  There was a slight merge
conflict, but I hope that's not related to this crash.  Perhaps the test
case is shaky and Josh's changes expose an underlying issue?

More info on the test case:

  # TEST: klp-convert symbols
  https://github.com/joe-lawrence/linux/blob/jp-v2-klp-convert/tools/testing/selftests/livepatch/test-livepatch.sh#L172

  which loads an ordinary kernel module (test_klp_convert_mod) and then
  this livepatch module, which contains references to both vmlinux
  symbols and a few found in the first module:
  https://github.com/joe-lawrence/linux/blob/jp-v2-klp-convert/lib/livepatch/test_klp_convert1.c

The livepatch's init function calls klp_enable_patch() as usual, and
then for testing purposes calls a few functions that required
klp-relocations.  I don't know if real livepatch modules would ever need
to do this from init code, but I did so just to make the test code
simpler.

This is what klp-convert created for klp-relocations:

  % readelf --wide --relocs test_klp_convert1.ko
  ...
  Relocation section '.klp.rela.vmlinux..text.unlikely' at offset 0x52000 contains 1 entry:
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  0000000000000002  000000340000001a R_390_GOTENT           0000000000000000 .klp.sym.vmlinux.saved_command_line,0 + 2

  Relocation section '.klp.rela.test_klp_convert_mod..text.unlikely' at offset 0x52018 contains 4 entries:
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  000000000000008a  0000003b00000014 R_390_PLT32DBL         0000000000000000 .klp.sym.test_klp_convert_mod.get_homonym_string,1 + 2
  000000000000006c  000000350000001a R_390_GOTENT           0000000000000000 .klp.sym.test_klp_convert_mod.homonym_string,1 + 2
  0000000000000042  0000003e00000014 R_390_PLT32DBL         0000000000000000 .klp.sym.test_klp_convert_mod.test_klp_get_driver_name,0 + 2
  0000000000000024  000000380000001a R_390_GOTENT           0000000000000000 .klp.sym.test_klp_convert_mod.driver_name,0 + 2

I can rework the test case to simplify and debug few things.  Let me
know if you have any specific ideas in mind.
 
-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 14:12             ` Josh Poimboeuf
@ 2020-04-23 18:10               ` Josh Poimboeuf
  2020-04-23 23:26                 ` Josh Poimboeuf
  2020-04-30 14:38                 ` Joe Lawrence
  0 siblings, 2 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-23 18:10 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Gerald Schaefer, live-patching, linux-kernel, Peter Zijlstra,
	Jessica Yu, linux-s390, heiko.carstens, Vasily Gorbik,
	Joe Lawrence

On Thu, Apr 23, 2020 at 09:12:28AM -0500, Josh Poimboeuf wrote:
> > > this is strange. While I would have expected an exception similar to
> > > this, it really should have happened on the "sturg" instruction which
> > > does the DAT-off store in s390_kernel_write(), and certainly not with
> > > an ID of 0004 (protection). However, in your case, it happens on a
> > > normal store instruction, with 0004 indicating a protection exception.
> > > 
> > > This is more like what I would expect e.g. in the case where you do
> > > _not_ use the s390_kernel_write() function for RO module text patching,
> > > but rather normal memory access. So I am pretty sure that this is not
> > > related to the s390_kernel_write(), but some other issue, maybe some
> > > place left where you still use normal memory access?
> > 
> > The call trace above also suggests that it is not a late relocation, no? 
> > The path is from KLP module init function through klp_enable_patch. It should 
> > mean that the to-be-patched object is loaded (it must be a module thanks 
> > to a check klp_init_object_loaded(), vmlinux relocations were processed 
> > earlier in apply_relocations()).
> > 
> > However, the KLP module state here must be COMING, so s390_kernel_write() 
> > should be used. What are we missing?
> 
> I'm also scratching my head.  It _should_ be using s390_kernel_write()
> based on the module state, but I don't see that on the stack trace.
> 
> This trace (and Gerald's comment) seem to imply it's using
> __builtin_memcpy(), which might expected for UNFORMED state.
> 
> Weird...

Mystery solved:

  $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
  apply_rela+0x16a/0x520:
  apply_rela at arch/s390/kernel/module.c:336

which corresponds to the following code in apply_rela():


	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
		if (info->plt_initialized == 0) {
			unsigned int *ip;
			ip = me->core_layout.base + me->arch.plt_offset +
				info->plt_offset;
			ip[0] = 0x0d10e310;	/* basr 1,0  */
			ip[1] = 0x100a0004;	/* lg	1,10(1) */


Notice how it's writing directly to text... oops.

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 18:10               ` Josh Poimboeuf
@ 2020-04-23 23:26                 ` Josh Poimboeuf
  2020-04-24  2:35                   ` Joe Lawrence
  2020-04-24  7:20                   ` Christian Borntraeger
  2020-04-30 14:38                 ` Joe Lawrence
  1 sibling, 2 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-23 23:26 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Gerald Schaefer, live-patching, linux-kernel, Peter Zijlstra,
	Jessica Yu, linux-s390, heiko.carstens, Vasily Gorbik,
	Joe Lawrence

On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 23, 2020 at 09:12:28AM -0500, Josh Poimboeuf wrote:
> > > > this is strange. While I would have expected an exception similar to
> > > > this, it really should have happened on the "sturg" instruction which
> > > > does the DAT-off store in s390_kernel_write(), and certainly not with
> > > > an ID of 0004 (protection). However, in your case, it happens on a
> > > > normal store instruction, with 0004 indicating a protection exception.
> > > > 
> > > > This is more like what I would expect e.g. in the case where you do
> > > > _not_ use the s390_kernel_write() function for RO module text patching,
> > > > but rather normal memory access. So I am pretty sure that this is not
> > > > related to the s390_kernel_write(), but some other issue, maybe some
> > > > place left where you still use normal memory access?
> > > 
> > > The call trace above also suggests that it is not a late relocation, no? 
> > > The path is from KLP module init function through klp_enable_patch. It should 
> > > mean that the to-be-patched object is loaded (it must be a module thanks 
> > > to a check klp_init_object_loaded(), vmlinux relocations were processed 
> > > earlier in apply_relocations()).
> > > 
> > > However, the KLP module state here must be COMING, so s390_kernel_write() 
> > > should be used. What are we missing?
> > 
> > I'm also scratching my head.  It _should_ be using s390_kernel_write()
> > based on the module state, but I don't see that on the stack trace.
> > 
> > This trace (and Gerald's comment) seem to imply it's using
> > __builtin_memcpy(), which might expected for UNFORMED state.
> > 
> > Weird...
> 
> Mystery solved:
> 
>   $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
>   apply_rela+0x16a/0x520:
>   apply_rela at arch/s390/kernel/module.c:336
> 
> which corresponds to the following code in apply_rela():
> 
> 
> 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
> 		if (info->plt_initialized == 0) {
> 			unsigned int *ip;
> 			ip = me->core_layout.base + me->arch.plt_offset +
> 				info->plt_offset;
> 			ip[0] = 0x0d10e310;	/* basr 1,0  */
> 			ip[1] = 0x100a0004;	/* lg	1,10(1) */
> 
> 
> Notice how it's writing directly to text... oops.

Here's a fix, using write() for the PLT and the GOT.

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index 2798329ebb74..fe446f42818f 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 
 			gotent = me->core_layout.base + me->arch.got_offset +
 				info->got_offset;
-			*gotent = val;
+			write(gotent, &val, sizeof(*gotent));
 			info->got_initialized = 1;
 		}
 		val = info->got_offset + rela->r_addend;
@@ -330,25 +330,29 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_PLTOFF32:	/* 32 bit offset from GOT to PLT. */
 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
 		if (info->plt_initialized == 0) {
-			unsigned int *ip;
+			unsigned int *ip, insn[5];
+
 			ip = me->core_layout.base + me->arch.plt_offset +
 				info->plt_offset;
-			ip[0] = 0x0d10e310;	/* basr 1,0  */
-			ip[1] = 0x100a0004;	/* lg	1,10(1) */
+
+			insn[0] = 0x0d10e310;	/* basr 1,0  */
+			insn[1] = 0x100a0004;	/* lg	1,10(1) */
 			if (IS_ENABLED(CONFIG_EXPOLINE) && !nospec_disable) {
 				unsigned int *ij;
 				ij = me->core_layout.base +
 					me->arch.plt_offset +
 					me->arch.plt_size - PLT_ENTRY_SIZE;
-				ip[2] = 0xa7f40000 +	/* j __jump_r1 */
+				insn[2] = 0xa7f40000 +	/* j __jump_r1 */
 					(unsigned int)(u16)
 					(((unsigned long) ij - 8 -
 					  (unsigned long) ip) / 2);
 			} else {
-				ip[2] = 0x07f10000;	/* br %r1 */
+				insn[2] = 0x07f10000;	/* br %r1 */
 			}
-			ip[3] = (unsigned int) (val >> 32);
-			ip[4] = (unsigned int) val;
+			insn[3] = (unsigned int) (val >> 32);
+			insn[4] = (unsigned int) val;
+
+			write(ip, insn, sizeof(insn));
 			info->plt_initialized = 1;
 		}
 		if (r_type == R_390_PLTOFF16 ||


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 23:26                 ` Josh Poimboeuf
@ 2020-04-24  2:35                   ` Joe Lawrence
  2020-04-24  4:12                     ` Josh Poimboeuf
  2020-04-24  7:20                   ` Christian Borntraeger
  1 sibling, 1 reply; 38+ messages in thread
From: Joe Lawrence @ 2020-04-24  2:35 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Miroslav Benes, Gerald Schaefer, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik

On Thu, Apr 23, 2020 at 06:26:57PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
> > Mystery solved:
> > 
> >   $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
> >   apply_rela+0x16a/0x520:
> >   apply_rela at arch/s390/kernel/module.c:336
> > 
> > which corresponds to the following code in apply_rela():
> > 
> > 
> > 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
> > 		if (info->plt_initialized == 0) {
> > 			unsigned int *ip;
> > 			ip = me->core_layout.base + me->arch.plt_offset +
> > 				info->plt_offset;
> > 			ip[0] = 0x0d10e310;	/* basr 1,0  */
> > 			ip[1] = 0x100a0004;	/* lg	1,10(1) */
> > 
> > 
> > Notice how it's writing directly to text... oops.
> 
> Here's a fix, using write() for the PLT and the GOT.
> 
> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> index 2798329ebb74..fe446f42818f 100644
> --- a/arch/s390/kernel/module.c
> +++ b/arch/s390/kernel/module.c
> @@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
>  
>  			gotent = me->core_layout.base + me->arch.got_offset +
>  				info->got_offset;
> -			*gotent = val;
> +			write(gotent, &val, sizeof(*gotent));
>  			info->got_initialized = 1;
>  		}
>  		val = info->got_offset + rela->r_addend;
> @@ -330,25 +330,29 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
>  	case R_390_PLTOFF32:	/* 32 bit offset from GOT to PLT. */
>  	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
>  		if (info->plt_initialized == 0) {
> -			unsigned int *ip;
> +			unsigned int *ip, insn[5];
> +
>  			ip = me->core_layout.base + me->arch.plt_offset +
>  				info->plt_offset;

Would it be too paranoid to declare:

  			const unsigned int *ip = me->core_layout.base + 
						 me->arch.plt_offset +
  						 info->plt_offset;

That would trip an assignment to read-only error if someone were tempted
to write directly through the pointer in the future.  Ditto for Elf_Addr
*gotent pointer in the R_390_GOTPLTENT case.

-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-24  2:35                   ` Joe Lawrence
@ 2020-04-24  4:12                     ` Josh Poimboeuf
  0 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-24  4:12 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Miroslav Benes, Gerald Schaefer, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik

On Thu, Apr 23, 2020 at 10:35:21PM -0400, Joe Lawrence wrote:
> > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> > index 2798329ebb74..fe446f42818f 100644
> > --- a/arch/s390/kernel/module.c
> > +++ b/arch/s390/kernel/module.c
> > @@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
> >  
> >  			gotent = me->core_layout.base + me->arch.got_offset +
> >  				info->got_offset;
> > -			*gotent = val;
> > +			write(gotent, &val, sizeof(*gotent));
> >  			info->got_initialized = 1;
> >  		}
> >  		val = info->got_offset + rela->r_addend;
> > @@ -330,25 +330,29 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
> >  	case R_390_PLTOFF32:	/* 32 bit offset from GOT to PLT. */
> >  	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
> >  		if (info->plt_initialized == 0) {
> > -			unsigned int *ip;
> > +			unsigned int *ip, insn[5];
> > +
> >  			ip = me->core_layout.base + me->arch.plt_offset +
> >  				info->plt_offset;
> 
> Would it be too paranoid to declare:
> 
>   			const unsigned int *ip = me->core_layout.base + 
> 						 me->arch.plt_offset +
>   						 info->plt_offset;
> 
> That would trip an assignment to read-only error if someone were tempted
> to write directly through the pointer in the future.  Ditto for Elf_Addr
> *gotent pointer in the R_390_GOTPLTENT case.

The only problem is then the write() triggers a warning because then we
*are* trying to write through the pointer :-)

arch/s390/kernel/module.c:300:10: warning: passing argument 1 of ‘write’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  300 |    write(gotent, &val, sizeof(*gotent));

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 23:26                 ` Josh Poimboeuf
  2020-04-24  2:35                   ` Joe Lawrence
@ 2020-04-24  7:20                   ` Christian Borntraeger
  2020-04-24 13:37                     ` Josh Poimboeuf
  1 sibling, 1 reply; 38+ messages in thread
From: Christian Borntraeger @ 2020-04-24  7:20 UTC (permalink / raw)
  To: Josh Poimboeuf, Miroslav Benes
  Cc: Gerald Schaefer, live-patching, linux-kernel, Peter Zijlstra,
	Jessica Yu, linux-s390, heiko.carstens, Vasily Gorbik,
	Joe Lawrence


On 24.04.20 01:26, Josh Poimboeuf wrote:
> On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
>> On Thu, Apr 23, 2020 at 09:12:28AM -0500, Josh Poimboeuf wrote:
>>>>> this is strange. While I would have expected an exception similar to
>>>>> this, it really should have happened on the "sturg" instruction which
>>>>> does the DAT-off store in s390_kernel_write(), and certainly not with
>>>>> an ID of 0004 (protection). However, in your case, it happens on a
>>>>> normal store instruction, with 0004 indicating a protection exception.
>>>>>
>>>>> This is more like what I would expect e.g. in the case where you do
>>>>> _not_ use the s390_kernel_write() function for RO module text patching,
>>>>> but rather normal memory access. So I am pretty sure that this is not
>>>>> related to the s390_kernel_write(), but some other issue, maybe some
>>>>> place left where you still use normal memory access?
>>>>
>>>> The call trace above also suggests that it is not a late relocation, no? 
>>>> The path is from KLP module init function through klp_enable_patch. It should 
>>>> mean that the to-be-patched object is loaded (it must be a module thanks 
>>>> to a check klp_init_object_loaded(), vmlinux relocations were processed 
>>>> earlier in apply_relocations()).
>>>>
>>>> However, the KLP module state here must be COMING, so s390_kernel_write() 
>>>> should be used. What are we missing?
>>>
>>> I'm also scratching my head.  It _should_ be using s390_kernel_write()
>>> based on the module state, but I don't see that on the stack trace.
>>>
>>> This trace (and Gerald's comment) seem to imply it's using
>>> __builtin_memcpy(), which might expected for UNFORMED state.
>>>
>>> Weird...
>>
>> Mystery solved:
>>
>>   $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
>>   apply_rela+0x16a/0x520:
>>   apply_rela at arch/s390/kernel/module.c:336
>>
>> which corresponds to the following code in apply_rela():
>>
>>
>> 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
>> 		if (info->plt_initialized == 0) {
>> 			unsigned int *ip;
>> 			ip = me->core_layout.base + me->arch.plt_offset +
>> 				info->plt_offset;
>> 			ip[0] = 0x0d10e310;	/* basr 1,0  */
>> 			ip[1] = 0x100a0004;	/* lg	1,10(1) */
>>
>>
>> Notice how it's writing directly to text... oops.
> 
> Here's a fix, using write() for the PLT and the GOT.

Are you going to provide a proper patch?

> 
> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> index 2798329ebb74..fe446f42818f 100644
> --- a/arch/s390/kernel/module.c
> +++ b/arch/s390/kernel/module.c
> @@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
>  
>  			gotent = me->core_layout.base + me->arch.got_offset +
>  				info->got_offset;
> -			*gotent = val;
> +			write(gotent, &val, sizeof(*gotent));
>  			info->got_initialized = 1;
>  		}
>  		val = info->got_offset + rela->r_addend;
> @@ -330,25 +330,29 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
>  	case R_390_PLTOFF32:	/* 32 bit offset from GOT to PLT. */
>  	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
>  		if (info->plt_initialized == 0) {
> -			unsigned int *ip;
> +			unsigned int *ip, insn[5];
> +
>  			ip = me->core_layout.base + me->arch.plt_offset +
>  				info->plt_offset;
> -			ip[0] = 0x0d10e310;	/* basr 1,0  */
> -			ip[1] = 0x100a0004;	/* lg	1,10(1) */
> +
> +			insn[0] = 0x0d10e310;	/* basr 1,0  */
> +			insn[1] = 0x100a0004;	/* lg	1,10(1) */
>  			if (IS_ENABLED(CONFIG_EXPOLINE) && !nospec_disable) {
>  				unsigned int *ij;
>  				ij = me->core_layout.base +
>  					me->arch.plt_offset +
>  					me->arch.plt_size - PLT_ENTRY_SIZE;
> -				ip[2] = 0xa7f40000 +	/* j __jump_r1 */
> +				insn[2] = 0xa7f40000 +	/* j __jump_r1 */
>  					(unsigned int)(u16)
>  					(((unsigned long) ij - 8 -
>  					  (unsigned long) ip) / 2);
>  			} else {
> -				ip[2] = 0x07f10000;	/* br %r1 */
> +				insn[2] = 0x07f10000;	/* br %r1 */
>  			}
> -			ip[3] = (unsigned int) (val >> 32);
> -			ip[4] = (unsigned int) val;
> +			insn[3] = (unsigned int) (val >> 32);
> +			insn[4] = (unsigned int) val;
> +
> +			write(ip, insn, sizeof(insn));
>  			info->plt_initialized = 1;
>  		}
>  		if (r_type == R_390_PLTOFF16 ||
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-24  7:20                   ` Christian Borntraeger
@ 2020-04-24 13:37                     ` Josh Poimboeuf
  0 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-24 13:37 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Miroslav Benes, Gerald Schaefer, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik, Joe Lawrence

On Fri, Apr 24, 2020 at 09:20:24AM +0200, Christian Borntraeger wrote:
> > Here's a fix, using write() for the PLT and the GOT.
> 
> Are you going to provide a proper patch?

Yes.  These incremental patches are intended to be part of the
discussion, feel free to ignore them.

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-22 12:28   ` Miroslav Benes
@ 2020-04-24 13:43     ` Josh Poimboeuf
  0 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-24 13:43 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: live-patching, linux-kernel, Peter Zijlstra, Jessica Yu,
	linux-s390, heiko.carstens

On Wed, Apr 22, 2020 at 02:28:26PM +0200, Miroslav Benes wrote:
> > +int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > +		       unsigned int symindex, unsigned int relsec,
> > +		       struct module *me)
> > +{
> > +	int ret;
> > +	bool early = me->state == MODULE_STATE_UNFORMED;
> > +	void *(*write)(void *, const void *, size_t) = memcpy;
> > +
> > +	if (!early) {
> > +		write = s390_kernel_write;
> > +		mutex_lock(&text_mutex);
> > +	}
> > +
> > +	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > +				   write);
> > +
> > +	if (!early)
> > +		mutex_unlock(&text_mutex);
> > +
> > +	return ret;
> > +}
> 
> It means that you can take text_mutex the second time here because it 
> is still taken in klp_init_object_loaded(). It is removed later in the 
> series though. The same applies for x86 patch.
> 
> Also, s390_kernel_write() grabs s390_kernel_write_lock spinlock before 
> writing anything, so maybe text_mutex is not really needed as long as 
> s390_kernel_write is called everywhere for text patching.

Good catch, maybe I'll just drop the text_mutex here.

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-23 18:10               ` Josh Poimboeuf
  2020-04-23 23:26                 ` Josh Poimboeuf
@ 2020-04-30 14:38                 ` Joe Lawrence
  2020-04-30 16:48                   ` Josh Poimboeuf
  1 sibling, 1 reply; 38+ messages in thread
From: Joe Lawrence @ 2020-04-30 14:38 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Miroslav Benes, Gerald Schaefer, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik

On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 23, 2020 at 09:12:28AM -0500, Josh Poimboeuf wrote:
> > > > this is strange. While I would have expected an exception similar to
> > > > this, it really should have happened on the "sturg" instruction which
> > > > does the DAT-off store in s390_kernel_write(), and certainly not with
> > > > an ID of 0004 (protection). However, in your case, it happens on a
> > > > normal store instruction, with 0004 indicating a protection exception.
> > > > 
> > > > This is more like what I would expect e.g. in the case where you do
> > > > _not_ use the s390_kernel_write() function for RO module text patching,
> > > > but rather normal memory access. So I am pretty sure that this is not
> > > > related to the s390_kernel_write(), but some other issue, maybe some
> > > > place left where you still use normal memory access?
> > > 
> > > The call trace above also suggests that it is not a late relocation, no? 
> > > The path is from KLP module init function through klp_enable_patch. It should 
> > > mean that the to-be-patched object is loaded (it must be a module thanks 
> > > to a check klp_init_object_loaded(), vmlinux relocations were processed 
> > > earlier in apply_relocations()).
> > > 
> > > However, the KLP module state here must be COMING, so s390_kernel_write() 
> > > should be used. What are we missing?
> > 
> > I'm also scratching my head.  It _should_ be using s390_kernel_write()
> > based on the module state, but I don't see that on the stack trace.
> > 
> > This trace (and Gerald's comment) seem to imply it's using
> > __builtin_memcpy(), which might expected for UNFORMED state.
> > 
> > Weird...
> 
> Mystery solved:
> 
>   $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
>   apply_rela+0x16a/0x520:
>   apply_rela at arch/s390/kernel/module.c:336
> 
> which corresponds to the following code in apply_rela():
> 
> 
> 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
> 		if (info->plt_initialized == 0) {
> 			unsigned int *ip;
> 			ip = me->core_layout.base + me->arch.plt_offset +
> 				info->plt_offset;
> 			ip[0] = 0x0d10e310;	/* basr 1,0  */
> 			ip[1] = 0x100a0004;	/* lg	1,10(1) */
> 
> 
> Notice how it's writing directly to text... oops.
> 

This is more of note for the future, but when/if we add livepatch
support on arm64 we'll need to make the very same adjustment there as
well.  See the following pattern:

arch/arm64/kernel/module.c:

  reloc_insn_movw()
  reloc_insn_imm()
  reloc_insn_adrp()

    *place = cpu_to_le32(insn);

maybe something like aarch64_insn_patch_text_nosync() could be used
there, I dunno. (It looks like ftrace and jump_labels are using that
interface.)

This is outside the scope of the patchset, but I thought I'd mention it
as I was curious to see how other arches were currently handling their
relocation updates.

-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-30 14:38                 ` Joe Lawrence
@ 2020-04-30 16:48                   ` Josh Poimboeuf
  2020-04-30 17:04                     ` Joe Lawrence
  0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2020-04-30 16:48 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Miroslav Benes, Gerald Schaefer, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik

On Thu, Apr 30, 2020 at 10:38:21AM -0400, Joe Lawrence wrote:
> On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
> > On Thu, Apr 23, 2020 at 09:12:28AM -0500, Josh Poimboeuf wrote:
> > > > > this is strange. While I would have expected an exception similar to
> > > > > this, it really should have happened on the "sturg" instruction which
> > > > > does the DAT-off store in s390_kernel_write(), and certainly not with
> > > > > an ID of 0004 (protection). However, in your case, it happens on a
> > > > > normal store instruction, with 0004 indicating a protection exception.
> > > > > 
> > > > > This is more like what I would expect e.g. in the case where you do
> > > > > _not_ use the s390_kernel_write() function for RO module text patching,
> > > > > but rather normal memory access. So I am pretty sure that this is not
> > > > > related to the s390_kernel_write(), but some other issue, maybe some
> > > > > place left where you still use normal memory access?
> > > > 
> > > > The call trace above also suggests that it is not a late relocation, no? 
> > > > The path is from KLP module init function through klp_enable_patch. It should 
> > > > mean that the to-be-patched object is loaded (it must be a module thanks 
> > > > to a check klp_init_object_loaded(), vmlinux relocations were processed 
> > > > earlier in apply_relocations()).
> > > > 
> > > > However, the KLP module state here must be COMING, so s390_kernel_write() 
> > > > should be used. What are we missing?
> > > 
> > > I'm also scratching my head.  It _should_ be using s390_kernel_write()
> > > based on the module state, but I don't see that on the stack trace.
> > > 
> > > This trace (and Gerald's comment) seem to imply it's using
> > > __builtin_memcpy(), which might expected for UNFORMED state.
> > > 
> > > Weird...
> > 
> > Mystery solved:
> > 
> >   $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
> >   apply_rela+0x16a/0x520:
> >   apply_rela at arch/s390/kernel/module.c:336
> > 
> > which corresponds to the following code in apply_rela():
> > 
> > 
> > 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
> > 		if (info->plt_initialized == 0) {
> > 			unsigned int *ip;
> > 			ip = me->core_layout.base + me->arch.plt_offset +
> > 				info->plt_offset;
> > 			ip[0] = 0x0d10e310;	/* basr 1,0  */
> > 			ip[1] = 0x100a0004;	/* lg	1,10(1) */
> > 
> > 
> > Notice how it's writing directly to text... oops.
> > 
> 
> This is more of note for the future, but when/if we add livepatch
> support on arm64 we'll need to make the very same adjustment there as
> well.  See the following pattern:
> 
> arch/arm64/kernel/module.c:
> 
>   reloc_insn_movw()
>   reloc_insn_imm()
>   reloc_insn_adrp()
> 
>     *place = cpu_to_le32(insn);
> 
> maybe something like aarch64_insn_patch_text_nosync() could be used
> there, I dunno. (It looks like ftrace and jump_labels are using that
> interface.)
> 
> This is outside the scope of the patchset, but I thought I'd mention it
> as I was curious to see how other arches were currently handling their
> relocation updates.

True... I suspect your klp-convert selftests will catch that?

-- 
Josh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
  2020-04-30 16:48                   ` Josh Poimboeuf
@ 2020-04-30 17:04                     ` Joe Lawrence
  0 siblings, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-30 17:04 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Miroslav Benes, Gerald Schaefer, live-patching, linux-kernel,
	Peter Zijlstra, Jessica Yu, linux-s390, heiko.carstens,
	Vasily Gorbik

On 4/30/20 12:48 PM, Josh Poimboeuf wrote:
> On Thu, Apr 30, 2020 at 10:38:21AM -0400, Joe Lawrence wrote:
>> On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
>> This is more of note for the future, but when/if we add livepatch
>> support on arm64 we'll need to make the very same adjustment there as
>> well.  See the following pattern:
>>
>> arch/arm64/kernel/module.c:
>>
>>    reloc_insn_movw()
>>    reloc_insn_imm()
>>    reloc_insn_adrp()
>>
>>      *place = cpu_to_le32(insn);
>>
>> maybe something like aarch64_insn_patch_text_nosync() could be used
>> there, I dunno. (It looks like ftrace and jump_labels are using that
>> interface.)
>>
>> This is outside the scope of the patchset, but I thought I'd mention it
>> as I was curious to see how other arches were currently handling their
>> relocation updates.
> 
> True... I suspect your klp-convert selftests will catch that?
> 

Indeed.  Actually I had hacked enough livepatch code support on ARM to 
see what happened when converting and loading the test patches :)

-- Joe


^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2020-04-30 17:04 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 14:04 [PATCH v2 0/9] livepatch,module: Remove .klp.arch and module_disable_ro() Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 1/9] livepatch: Disallow vmlinux.ko Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early Josh Poimboeuf
2020-04-20 17:57   ` Joe Lawrence
2020-04-20 18:25     ` Josh Poimboeuf
2020-04-20 19:01       ` Joe Lawrence
2020-04-20 19:11         ` Josh Poimboeuf
2020-04-20 19:49           ` Joe Lawrence
2020-04-20 19:51             ` Josh Poimboeuf
2020-04-23  1:10           ` Joe Lawrence
2020-04-21 11:54     ` Miroslav Benes
2020-04-17 14:04 ` [PATCH v2 3/9] livepatch: Remove .klp.arch Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 4/9] livepatch: Prevent module-specific KLP rela sections from referencing vmlinux symbols Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 5/9] s390: Change s390_kernel_write() return type to match memcpy() Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations Josh Poimboeuf
2020-04-22 12:28   ` Miroslav Benes
2020-04-24 13:43     ` Josh Poimboeuf
2020-04-22 14:40   ` Gerald Schaefer
2020-04-22 15:21     ` Gerald Schaefer
2020-04-22 19:46       ` Josh Poimboeuf
2020-04-23 12:33         ` Gerald Schaefer
2020-04-23 13:22           ` Miroslav Benes
2020-04-23 14:12             ` Josh Poimboeuf
2020-04-23 18:10               ` Josh Poimboeuf
2020-04-23 23:26                 ` Josh Poimboeuf
2020-04-24  2:35                   ` Joe Lawrence
2020-04-24  4:12                     ` Josh Poimboeuf
2020-04-24  7:20                   ` Christian Borntraeger
2020-04-24 13:37                     ` Josh Poimboeuf
2020-04-30 14:38                 ` Joe Lawrence
2020-04-30 16:48                   ` Josh Poimboeuf
2020-04-30 17:04                     ` Joe Lawrence
2020-04-23 14:21             ` Joe Lawrence
2020-04-17 14:04 ` [PATCH v2 7/9] x86/module: Use text_poke() " Josh Poimboeuf
2020-04-17 14:29   ` Peter Zijlstra
2020-04-17 14:51     ` Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 8/9] livepatch: Remove module_disable_ro() usage Josh Poimboeuf
2020-04-17 14:04 ` [PATCH v2 9/9] module: Remove module_disable_ro() Josh Poimboeuf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).