livepatch: Clear relocation targets on a module removal

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

He also proposed three different solutions. We could remove the error check in apply_relocate_add() introduced by commit eda9cec4c9a1 ("x86/module: Detect and skip invalid relocations"). However the check is useful for detecting corrupted modules.

We could also deny the patched modules to be removed. If it proved to be a major drawback for users, we could still implement a different approach. The solution would also complicate the existing code a lot.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

Signed-off-by Miroslav Benes <[email protected]>

Signed-off-by Song Liu <[email protected]>

Reported-by Josh Poimboeuf <[email protected]>

Tested-by Joe Lawrence <[email protected]> # x86_64, s390x, ppc64le

---

NOTE: powerpc32 code is only compile tested.

Changes v6 = v7:
1. Reduce code duplication in livepatch/core.c and x86/kernel/module.c.
2. Add more comments to powerpc/kernel/module_64.c.
3. Added Joe's Tested-by (which I should have added in v6).

Changes v5 = v6:
1. Fix powerpc64.
2. Fix compile for powerpc32.

Changes v4 = v5:
1. Fix compile with powerpc.

Changes v3 = v4:
1. Reuse __apply_relocate_add to make it more reliable in long term. (Josh Poimboeuf)
2. Add back ppc64 logic from v2, with changes to match current code. (Josh Poimboeuf)

Changes v2 => v3:
1. Rewrite x86 changes to match current code style.
2. Remove powerpc changes as there is no test coverage in v3.
3. Only keep 1/3 of v2.

v2: https://lore.kernel.org/all/[email protected]/T/#u

---
 arch/powerpc/kernel/module_32.c |  10 +++
 arch/powerpc/kernel/module_64.c |  61 ++++++++++++++++++
 arch/s390/kernel/module.c       |   8 +++
 arch/x86/kernel/module.c        | 108 ++++++++++++++++++++++----------
 include/linux/moduleloader.h    |   7 +++
 kernel/livepatch/core.c         |  85 ++++++++++++++++---------
 6 files changed, 217 insertions(+), 62 deletions(-)

arch/powerpc/kernel/module_32.c +10 -0

index ea6536171778..e3c312770453 100644
@@ -285,6 +285,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
        return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf32_Shdr *sechdrs,
+                  const char *strtab,
+                  unsigned int symindex,
+                  unsigned int relsec,
+                  struct module *me)
+{
+}
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 notrace int module_trampoline_target(struct module *mod, unsigned long addr,
                                     unsigned long *target)

arch/powerpc/kernel/module_64.c +61 -0

index 7e45dc98df8a..83e6c226009c 100644
@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
        return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                      const char *strtab,
+                      unsigned int symindex,
+                      unsigned int relsec,
+                      struct module *me)
+{
+       unsigned int i;
+       Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+       Elf64_Sym *sym;
+       unsigned long *location;
+       const char *symname;
+       u32 *instruction;
+
+       pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+                sechdrs[relsec].sh_info);
+
+       for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+               location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+                       + rela[i].r_offset;
+               sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                       + ELF64_R_SYM(rela[i].r_info);
+               symname = me->core_kallsyms.strtab
+                       + sym->st_name;
+
+               if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                       continue;
+               /*
+                * reverse the operations in apply_relocate_add() for case
+                * R_PPC_REL24.
+                */
+               if (sym->st_shndx != SHN_UNDEF &&
+                   sym->st_shndx != SHN_LIVEPATCH)
+                       continue;
+
+               /* skip mprofile and ftrace calls, same as restore_r2() */
+               if (is_mprofile_ftrace_call(symname))
+                       continue;
+
+               instruction = (u32 *)location;
+               /* skip sibling call, same as restore_r2() */
+               if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+                       continue;
+
+               instruction += 1;
+               /*
+                * Patch location + 1 back to NOP so the next
+                * apply_relocate_add() call (reload the module) will not
+                * fail the sanity check in restore_r2():
+                *
+                *         if (*instruction != PPC_RAW_NOP()) {
+                *             pr_err(...);
+                *             return 0;
+                *         }
+                */
+               patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
+       }
+
+}
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 int module_trampoline_target(struct module *mod, unsigned long addr,
                             unsigned long *target)

arch/s390/kernel/module.c +8 -0

index 2d159b32885b..cc6784fbc1ac 100644
@@ -500,6 +500,14 @@ static int module_alloc_ftrace_hotpatch_trampolines(struct module *me,
 }
 #endif /* CONFIG_FUNCTION_TRACER */
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs, const char *strtab,
+                       unsigned int symindex, unsigned int relsec,
+                       struct module *me)
+{
+}
+#endif
+
 int module_finalize(const Elf_Ehdr *hdr,
                    const Elf_Shdr *sechdrs,
                    struct module *me)

arch/x86/kernel/module.c +76 -32

index c032edcd3d95..8f997959e526 100644
@@ -128,18 +128,20 @@ int apply_relocate(Elf32_Shdr *sechdrs,
        return 0;
 }
 #else /*X86_64*/
-static int __apply_relocate_add(Elf64_Shdr *sechdrs,
+static int __write_relocate_add(Elf64_Shdr *sechdrs,
                   const char *strtab,
                   unsigned int symindex,
                   unsigned int relsec,
                   struct module *me,
-                  void *(*write)(void *dest, const void *src, size_t len))
+                  void *(*write)(void *dest, const void *src, size_t len),
+                  bool apply)
 {
        unsigned int i;
        Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr;
        Elf64_Sym *sym;
        void *loc;
        u64 val;
+       u64 zero = 0ULL;
 
        DEBUGP("Applying relocate section %u to %u\n",
               relsec, sechdrs[relsec].sh_info);

@@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
                case R_X86_64_NONE:
                        break;
                case R_X86_64_64:
-                       if (*(u64 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 8);
+                       if (apply) {
+                               if (*(u64 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 8);
+                       } else {
+                               write(loc, &zero, 8);
+                       }
                        break;
                case R_X86_64_32:
-                       if (*(u32 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 4);
-                       if (val != *(u32 *)loc)
-                               goto overflow;
+                       if (apply) {
+                               if (*(u32 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 4);
+                               if (val != *(u32 *)loc)
+                                       goto overflow;
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_32S:
-                       if (*(s32 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 4);
-                       if ((s64)val != *(s32 *)loc)
-                               goto overflow;
+                       if (apply) {
+                               if (*(s32 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 4);
+                               if ((s64)val != *(s32 *)loc)
+                                       goto overflow;
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_PC32:
                case R_X86_64_PLT32:
-                       if (*(u32 *)loc != 0)
-                               goto invalid_relocation;
-                       val -= (u64)loc;
-                       write(loc, &val, 4);
+                       if (apply) {
+                               if (*(u32 *)loc != 0)
+                                       goto invalid_relocation;
+                               val -= (u64)loc;
+                               write(loc, &val, 4);
 #if 0
-                       if ((s64)val != *(s32 *)loc)
-                               goto overflow;
+                               if ((s64)val != *(s32 *)loc)
+                                       goto overflow;
 #endif
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_PC64:
-                       if (*(u64 *)loc != 0)
-                               goto invalid_relocation;
-                       val -= (u64)loc;
-                       write(loc, &val, 8);
+                       if (apply) {
+                               if (*(u64 *)loc != 0)
+                                       goto invalid_relocation;
+                               val -= (u64)loc;
+                               write(loc, &val, 8);
+                       } else {
+                               write(loc, &zero, 8);
+                       }
                        break;
                default:
                        pr_err("%s: Unknown rela relocation: %llu\n",

@@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
        return -ENOEXEC;
 }
 
-int apply_relocate_add(Elf64_Shdr *sechdrs,
-                  const char *strtab,
-                  unsigned int symindex,
-                  unsigned int relsec,
-                  struct module *me)
+static int write_relocate_add(Elf64_Shdr *sechdrs,
+                             const char *strtab,
+                             unsigned int symindex,
+                             unsigned int relsec,
+                             struct module *me,
+                             bool apply)
 {
        int ret;
        bool early = me->state == MODULE_STATE_UNFORMED;

@@ -234,8 +257,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
                mutex_lock(&text_mutex);
        }
 
-       ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
-                                  write);
+       ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
+                                  write, apply);
 
        if (!early) {
                text_poke_sync();

@@ -245,6 +268,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
        return ret;
 }
 
+int apply_relocate_add(Elf64_Shdr *sechdrs,
+                  const char *strtab,
+                  unsigned int symindex,
+                  unsigned int relsec,
+                  struct module *me)
+{
+       return write_relocate_add(sechdrs, strtab, symindex, relsec, me, true);
+}
+
+#ifdef CONFIG_LIVEPATCH
+
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                       const char *strtab,
+                       unsigned int symindex,
+                       unsigned int relsec,
+                       struct module *me)
+{
+       write_relocate_add(sechdrs, strtab, symindex, relsec, me, false);
+}
+#endif
+
 #endif
 
 int module_finalize(const Elf_Ehdr *hdr,

include/linux/moduleloader.h +7 -0

index 9e09d11ffe5b..958e6da7f475 100644
@@ -72,6 +72,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
                       unsigned int symindex,
                       unsigned int relsec,
                       struct module *mod);
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf_Shdr *sechdrs,
+                  const char *strtab,
+                  unsigned int symindex,
+                  unsigned int relsec,
+                  struct module *me);
+#endif
 #else
 static inline int apply_relocate_add(Elf_Shdr *sechdrs,
                                     const char *strtab,

kernel/livepatch/core.c +55 -31

index 9ada0bc5247b..8cd04643f988 100644
@@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
        return 0;
 }
 
+static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+                                   const char *shstrtab, const char *strtab,
+                                   unsigned int symndx, unsigned int secndx,
+                                   const char *objname, bool apply)
+{
+       int cnt, ret;
+       char sec_objname[MODULE_NAME_LEN];
+       Elf_Shdr *sec = sechdrs + secndx;
+
+       /*
+        * 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;
+       }
+
+       if (strcmp(objname ? objname : "vmlinux", sec_objname))
+               return 0;
+
+       ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
+       if (ret)
+               return ret;
+
+       if (apply)
+               return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+       clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+       return 0;
+}
+
 /*
  * 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

@@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
                             unsigned int symndx, unsigned int secndx,
                             const char *objname)
 {
-       int cnt, ret;
-       char sec_objname[MODULE_NAME_LEN];
-       Elf_Shdr *sec = sechdrs + secndx;
-
-       /*
-        * 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;
-       }
-
-       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, secndx, pmod);
+       return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
+                                       secndx, objname, true);
 }
 
 /*

@@ -762,8 +774,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
                           func->old_sympos ? func->old_sympos : 1);
 }
 
-static int klp_apply_object_relocs(struct klp_patch *patch,
-                                  struct klp_object *obj)
+static int klp_write_object_relocs(struct klp_patch *patch,
+                                  struct klp_object *obj,
+                                  bool apply)
 {
        int i, ret;
        struct klp_modinfo *info = patch->mod->klp_info;

@@ -774,10 +787,10 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
                if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
                        continue;
 
-               ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
+               ret = klp_write_section_relocs(patch->mod, info->sechdrs,
                                               info->secstrings,
                                               patch->mod->core_kallsyms.strtab,
-                                              info->symndx, i, obj->name);
+                                              info->symndx, i, obj->name, apply);
                if (ret)
                        return ret;
        }

@@ -785,6 +798,18 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
        return 0;
 }
 
+static int klp_apply_object_relocs(struct klp_patch *patch,
+                                  struct klp_object *obj)
+{
+       return klp_write_object_relocs(patch, obj, true);
+}
+
+static void klp_clear_object_relocs(struct klp_patch *patch,
+                                   struct klp_object *obj)
+{
+       klp_write_object_relocs(patch, obj, false);
+}
+
 /* 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)

@@ -1172,7 +1197,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
                        klp_unpatch_object(obj);
 
                        klp_post_unpatch_callback(obj);
-
+                       klp_clear_object_relocs(patch, obj);
                        klp_free_object_loaded(obj);
                        break;
                }
--

2.30.2

Hi folks,

Happy New Year!

Could you please share your comments/suggestions on this version?

Thanks,
Song

Quoted 41 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

He also proposed three different solutions. We could remove the error check in apply_relocate_add() introduced by commit eda9cec4c9a1 ("x86/module: Detect and skip invalid relocations"). However the check is useful for detecting corrupted modules.

We could also deny the patched modules to be removed. If it proved to be a major drawback for users, we could still implement a different approach. The solution would also complicate the existing code a lot.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

Signed-off-by Miroslav Benes <[email protected]>

Signed-off-by Song Liu <[email protected]>

Reported-by Josh Poimboeuf <[email protected]>

Tested-by Joe Lawrence <[email protected]> # x86_64, s390x, ppc64le

Quoted 86 lines

---

NOTE: powerpc32 code is only compile tested.

Changes v6 = v7:
1. Reduce code duplication in livepatch/core.c and x86/kernel/module.c.
2. Add more comments to powerpc/kernel/module_64.c.
3. Added Joe's Tested-by (which I should have added in v6).

Changes v5 = v6:
1. Fix powerpc64.
2. Fix compile for powerpc32.

Changes v4 = v5:
1. Fix compile with powerpc.

Changes v3 = v4:
1. Reuse __apply_relocate_add to make it more reliable in long term. (Josh Poimboeuf)
2. Add back ppc64 logic from v2, with changes to match current code. (Josh Poimboeuf)

Changes v2 => v3:
1. Rewrite x86 changes to match current code style.
2. Remove powerpc changes as there is no test coverage in v3.
3. Only keep 1/3 of v2.

v2: https://lore.kernel.org/all/[email protected]/T/#u

---
 arch/powerpc/kernel/module_32.c |  10 +++
 arch/powerpc/kernel/module_64.c |  61 ++++++++++++++++++
 arch/s390/kernel/module.c       |   8 +++
 arch/x86/kernel/module.c        | 108 ++++++++++++++++++++++----------
 include/linux/moduleloader.h    |   7 +++
 kernel/livepatch/core.c         |  85 ++++++++++++++++---------
 6 files changed, 217 insertions(+), 62 deletions(-)

arch/powerpc/kernel/module_32.c +10 -0

index ea6536171778..e3c312770453 100644
@@ -285,6 +285,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
        return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf32_Shdr *sechdrs,
+                  const char *strtab,
+                  unsigned int symindex,
+                  unsigned int relsec,
+                  struct module *me)
+{
+}
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 notrace int module_trampoline_target(struct module *mod, unsigned long addr,
                                     unsigned long *target)

arch/powerpc/kernel/module_64.c +19 -0

index 7e45dc98df8a..83e6c226009c 100644
@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
        return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                      const char *strtab,
+                      unsigned int symindex,
+                      unsigned int relsec,
+                      struct module *me)
+{
+       unsigned int i;
+       Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+       Elf64_Sym *sym;
+       unsigned long *location;
+       const char *symname;
+       u32 *instruction;
+
+       pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+                sechdrs[relsec].sh_info);
+
+       for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+               location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 768

Quoted 8 lines

arch/powerpc/kernel/module_64.c +8 -0

+                       + rela[i].r_offset;
+               sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                       + ELF64_R_SYM(rela[i].r_info);
+               symname = me->core_kallsyms.strtab
+                       + sym->st_name;
+
+               if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                       continue;

Quoted 65 lines

arch/powerpc/kernel/module_64.c +34 -0

+               /*
+                * reverse the operations in apply_relocate_add() for case
+                * R_PPC_REL24.
+                */
+               if (sym->st_shndx != SHN_UNDEF &&
+                   sym->st_shndx != SHN_LIVEPATCH)
+                       continue;
+
+               /* skip mprofile and ftrace calls, same as restore_r2() */
+               if (is_mprofile_ftrace_call(symname))
+                       continue;
+
+               instruction = (u32 *)location;
+               /* skip sibling call, same as restore_r2() */
+               if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+                       continue;
+
+               instruction += 1;
+               /*
+                * Patch location + 1 back to NOP so the next
+                * apply_relocate_add() call (reload the module) will not
+                * fail the sanity check in restore_r2():
+                *
+                *         if (*instruction != PPC_RAW_NOP()) {
+                *             pr_err(...);
+                *             return 0;
+                *         }
+                */
+               patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
+       }
+
+}
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 int module_trampoline_target(struct module *mod, unsigned long addr,
                             unsigned long *target)

arch/s390/kernel/module.c +8 -0

index 2d159b32885b..cc6784fbc1ac 100644
@@ -500,6 +500,14 @@ static int module_alloc_ftrace_hotpatch_trampolines(struct module *me,
 }
 #endif /* CONFIG_FUNCTION_TRACER */
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs, const char *strtab,
+                       unsigned int symindex, unsigned int relsec,
+                       struct module *me)
+{
+}
+#endif
+
 int module_finalize(const Elf_Ehdr *hdr,
                    const Elf_Shdr *sechdrs,
                    struct module *me)

arch/x86/kernel/module.c +0 -1

index c032edcd3d95..8f997959e526 100644
@@ -128,18 +128,20 @@ int apply_relocate(Elf32_Shdr *sechdrs,
        return 0;
 }
 #else /*X86_64*/
-static int __apply_relocate_add(Elf64_Shdr *sechdrs,

arch/x86/kernel/module.c int apply_relocate(Elf32_Shdr *sechdrs, · line 137

Quoted 8 lines

arch/x86/kernel/module.c +3 -1

+static int __write_relocate_add(Elf64_Shdr *sechdrs,
                   const char *strtab,
                   unsigned int symindex,
                   unsigned int relsec,
                   struct module *me,
-                  void *(*write)(void *dest, const void *src, size_t len))
+                  void *(*write)(void *dest, const void *src, size_t len),
+                  bool apply)

arch/x86/kernel/module.c int apply_relocate(Elf32_Shdr *sechdrs, · line 147

Quoted 10 lines

arch/x86/kernel/module.c +1 -0

 {
        unsigned int i;
        Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr;
        Elf64_Sym *sym;
        void *loc;
        u64 val;
+       u64 zero = 0ULL;
 
        DEBUGP("Applying relocate section %u to %u\n",
               relsec, sechdrs[relsec].sh_info);

Quoted 56 lines

arch/x86/kernel/module.c +28 -17

@@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
                case R_X86_64_NONE:
                        break;
                case R_X86_64_64:
-                       if (*(u64 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 8);
+                       if (apply) {
+                               if (*(u64 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 8);
+                       } else {
+                               write(loc, &zero, 8);
+                       }
                        break;
                case R_X86_64_32:
-                       if (*(u32 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 4);
-                       if (val != *(u32 *)loc)
-                               goto overflow;
+                       if (apply) {
+                               if (*(u32 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 4);
+                               if (val != *(u32 *)loc)
+                                       goto overflow;
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_32S:
-                       if (*(s32 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 4);
-                       if ((s64)val != *(s32 *)loc)
-                               goto overflow;
+                       if (apply) {
+                               if (*(s32 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 4);
+                               if ((s64)val != *(s32 *)loc)
+                                       goto overflow;
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_PC32:
                case R_X86_64_PLT32:
-                       if (*(u32 *)loc != 0)
-                               goto invalid_relocation;
-                       val -= (u64)loc;
-                       write(loc, &val, 4);
+                       if (apply) {
+                               if (*(u32 *)loc != 0)
+                                       goto invalid_relocation;

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 208

Quoted 8 lines

arch/x86/kernel/module.c +4 -2

+                               val -= (u64)loc;
+                               write(loc, &val, 4);
 #if 0
-                       if ((s64)val != *(s32 *)loc)
-                               goto overflow;
+                               if ((s64)val != *(s32 *)loc)
+                                       goto overflow;
 #endif

Quoted 9 lines

arch/x86/kernel/module.c +3 -4

+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_PC64:
-                       if (*(u64 *)loc != 0)
-                               goto invalid_relocation;
-                       val -= (u64)loc;
-                       write(loc, &val, 8);

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 221

Quoted 8 lines

arch/x86/kernel/module.c +8 -0

+                       if (apply) {
+                               if (*(u64 *)loc != 0)
+                                       goto invalid_relocation;
+                               val -= (u64)loc;
+                               write(loc, &val, 8);
+                       } else {
+                               write(loc, &zero, 8);
+                       }

Quoted 205 lines

arch/x86/kernel/module.c +29 -7

                        break;
                default:
                        pr_err("%s: Unknown rela relocation: %llu\n",

@@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
        return -ENOEXEC;
 }
 
-int apply_relocate_add(Elf64_Shdr *sechdrs,
-                  const char *strtab,
-                  unsigned int symindex,
-                  unsigned int relsec,
-                  struct module *me)
+static int write_relocate_add(Elf64_Shdr *sechdrs,
+                             const char *strtab,
+                             unsigned int symindex,
+                             unsigned int relsec,
+                             struct module *me,
+                             bool apply)
 {
        int ret;
        bool early = me->state == MODULE_STATE_UNFORMED;

@@ -234,8 +257,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
                mutex_lock(&text_mutex);
        }
 
-       ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
-                                  write);
+       ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
+                                  write, apply);
 
        if (!early) {
                text_poke_sync();

@@ -245,6 +268,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
        return ret;
 }
 
+int apply_relocate_add(Elf64_Shdr *sechdrs,
+                  const char *strtab,
+                  unsigned int symindex,
+                  unsigned int relsec,
+                  struct module *me)
+{
+       return write_relocate_add(sechdrs, strtab, symindex, relsec, me, true);
+}
+
+#ifdef CONFIG_LIVEPATCH
+
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                       const char *strtab,
+                       unsigned int symindex,
+                       unsigned int relsec,
+                       struct module *me)
+{
+       write_relocate_add(sechdrs, strtab, symindex, relsec, me, false);
+}
+#endif
+
 #endif
 
 int module_finalize(const Elf_Ehdr *hdr,

include/linux/moduleloader.h +7 -0

index 9e09d11ffe5b..958e6da7f475 100644
@@ -72,6 +72,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
                       unsigned int symindex,
                       unsigned int relsec,
                       struct module *mod);
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf_Shdr *sechdrs,
+                  const char *strtab,
+                  unsigned int symindex,
+                  unsigned int relsec,
+                  struct module *me);
+#endif
 #else
 static inline int apply_relocate_add(Elf_Shdr *sechdrs,
                                     const char *strtab,

kernel/livepatch/core.c +54 -29

index 9ada0bc5247b..8cd04643f988 100644
@@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
        return 0;
 }
 
+static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+                                   const char *shstrtab, const char *strtab,
+                                   unsigned int symndx, unsigned int secndx,
+                                   const char *objname, bool apply)
+{
+       int cnt, ret;
+       char sec_objname[MODULE_NAME_LEN];
+       Elf_Shdr *sec = sechdrs + secndx;
+
+       /*
+        * 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;
+       }
+
+       if (strcmp(objname ? objname : "vmlinux", sec_objname))
+               return 0;
+
+       ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
+       if (ret)
+               return ret;
+
+       if (apply)
+               return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+       clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+       return 0;
+}
+
 /*
  * 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

@@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
                             unsigned int symndx, unsigned int secndx,
                             const char *objname)
 {
-       int cnt, ret;
-       char sec_objname[MODULE_NAME_LEN];
-       Elf_Shdr *sec = sechdrs + secndx;
-
-       /*
-        * 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;
-       }
-
-       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, secndx, pmod);
+       return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
+                                       secndx, objname, true);
 }
 
 /*

@@ -762,8 +774,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
                           func->old_sympos ? func->old_sympos : 1);
 }
 
-static int klp_apply_object_relocs(struct klp_patch *patch,
-                                  struct klp_object *obj)
+static int klp_write_object_relocs(struct klp_patch *patch,
+                                  struct klp_object *obj,
+                                  bool apply)
 {
        int i, ret;
        struct klp_modinfo *info = patch->mod->klp_info;

@@ -774,10 +787,10 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
                if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
                        continue;
 
-               ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
+               ret = klp_write_section_relocs(patch->mod, info->sechdrs,
                                               info->secstrings,
                                               patch->mod->core_kallsyms.strtab,
-                                              info->symndx, i, obj->name);
+                                              info->symndx, i, obj->name, apply);
                if (ret)
                        return ret;
        }

@@ -785,6 +798,18 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
        return 0;
 }
 
+static int klp_apply_object_relocs(struct klp_patch *patch,
+                                  struct klp_object *obj)
+{
+       return klp_write_object_relocs(patch, obj, true);
+}
+
+static void klp_clear_object_relocs(struct klp_patch *patch,
+                                   struct klp_object *obj)
+{
+       klp_write_object_relocs(patch, obj, false);
+}
+
 /* 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)

@@ -1172,7 +1197,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
                        klp_unpatch_object(obj);
 

Quoted 9 lines

kernel/livepatch/core.c +1 -2

                        klp_post_unpatch_callback(obj);
-
+                       klp_clear_object_relocs(patch, obj);
                        klp_free_object_loaded(obj);
                        break;
                }
--

2.30.2

Quoted 5 lines

Quoted 5 lines

Signed-off-by Miroslav Benes <[email protected]>

Signed-off-by Song Liu <[email protected]>

Reported-by Josh Poimboeuf <[email protected]>

Tested-by Joe Lawrence <[email protected]> # x86_64, s390x, ppc64le

Since there is still some work required for ppc64le and possibly s390x, let's strip the tested-by tag. Each version should be re-tested and then we can let the maintainer add it on the final version.

Quoted 19 lines

Quoted 19 lines

arch/powerpc/kernel/module_64.c +19 -0

+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                      const char *strtab,
+                      unsigned int symindex,
+                      unsigned int relsec,
+                      struct module *me)
+{
+       unsigned int i;
+       Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+       Elf64_Sym *sym;
+       unsigned long *location;
+       const char *symname;
+       u32 *instruction;
+
+       pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+                sechdrs[relsec].sh_info);
+
+       for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+               location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr

Quoted 8 lines

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 768

Quoted 8 lines

arch/powerpc/kernel/module_64.c +8 -0

+                       + rela[i].r_offset;
+               sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                       + ELF64_R_SYM(rela[i].r_info);
+               symname = me->core_kallsyms.strtab
+                       + sym->st_name;
+
+               if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                       continue;

Ppc64le will need to handle additional relocation types.

While debugging a related issue on ppc64le regarding CONFIG_STRICT_MODULE_RWX [3], these were the extent of the klp-relocation types generated by kpatch-build and klp-convert-tree:

- R_PPC64_REL24 to symbols in other .text sections
- R_PPC64_ADDR64 to symbols thru .TOC
- R_PPC64_REL64 to static key symbols

I believe R_PPC64_ADDR64 and R_PPC64_REL64 can be simply be reset to 0.

[3] https://github.com/linuxppc/issues/issues/375#issuecomment-1233698835

Quoted 8 lines

Quoted 8 lines

#else /*X86_64*/

arch/x86/kernel/module.c +1 -2

-static int __apply_relocate_add(Elf64_Shdr *sechdrs,
+static int __write_relocate_add(Elf64_Shdr *sechdrs,
                   const char *strtab,
                   unsigned int symindex,
                   unsigned int relsec,
                   struct module *me,
-                  void *(*write)(void *dest, const void *src, size_t len))

Quoted 2 lines

arch/x86/kernel/module.c int apply_relocate(Elf32_Shdr *sechdrs, · line 137

Quoted 2 lines

arch/x86/kernel/module.c +2 -0

+                  void *(*write)(void *dest, const void *src, size_t len),
+                  bool apply)

Aside: I do prefer the style of one function to handle applying/clearing of relocations. x86_64 isn't too bad, but other arches have a much richer set of relocations that do all sorts of relative/offset/TOC/etc gymnastics that keeping their code in one spot should be much more maintainable.

Quoted 9 lines

Quoted 9 lines

 {
        unsigned int i;
        Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr;
        Elf64_Sym *sym;
        void *loc;
        u64 val;

+ u64 zero = 0ULL;

Quoted 2 lines

Quoted 2 lines

DEBUGP("Applying relocate section %u to %u\n",
relsec, sechdrs[relsec].sh_info);

How about keying off the apply bool to display "Applying" vs "Clearing".

Quoted 57 lines

Quoted 57 lines

arch/x86/kernel/module.c +28 -17

@@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
                case R_X86_64_NONE:
                        break;
                case R_X86_64_64:
-                       if (*(u64 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 8);
+                       if (apply) {
+                               if (*(u64 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 8);
+                       } else {
+                               write(loc, &zero, 8);
+                       }
                        break;
                case R_X86_64_32:
-                       if (*(u32 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 4);
-                       if (val != *(u32 *)loc)
-                               goto overflow;
+                       if (apply) {
+                               if (*(u32 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 4);
+                               if (val != *(u32 *)loc)
+                                       goto overflow;
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_32S:
-                       if (*(s32 *)loc != 0)
-                               goto invalid_relocation;
-                       write(loc, &val, 4);
-                       if ((s64)val != *(s32 *)loc)
-                               goto overflow;
+                       if (apply) {
+                               if (*(s32 *)loc != 0)
+                                       goto invalid_relocation;
+                               write(loc, &val, 4);
+                               if ((s64)val != *(s32 *)loc)
+                                       goto overflow;
+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_PC32:
                case R_X86_64_PLT32:
-                       if (*(u32 *)loc != 0)
-                               goto invalid_relocation;
-                       val -= (u64)loc;
-                       write(loc, &val, 4);
+                       if (apply) {
+                               if (*(u32 *)loc != 0)
+                                       goto invalid_relocation;

Quoted 8 lines

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 208

Quoted 8 lines

arch/x86/kernel/module.c +4 -2

+                               val -= (u64)loc;
+                               write(loc, &val, 4);
 #if 0
-                       if ((s64)val != *(s32 *)loc)
-                               goto overflow;
+                               if ((s64)val != *(s32 *)loc)
+                                       goto overflow;
 #endif

Btw, This has been #if 0'd for so long I wonder if we should just remove it?

Quoted 9 lines

Quoted 9 lines

arch/x86/kernel/module.c +3 -4

+                       } else {
+                               write(loc, &zero, 4);
+                       }
                        break;
                case R_X86_64_PC64:
-                       if (*(u64 *)loc != 0)
-                               goto invalid_relocation;
-                       val -= (u64)loc;
-                       write(loc, &val, 8);

Quoted 8 lines

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 221

Quoted 8 lines

arch/x86/kernel/module.c +8 -0

+                       if (apply) {
+                               if (*(u64 *)loc != 0)
+                                       goto invalid_relocation;
+                               val -= (u64)loc;
+                               write(loc, &val, 8);
+                       } else {
+                               write(loc, &zero, 8);
+                       }

In my branch [2] ("(song, x86_64 suggestions) livepatch: Clear relocation targets on a module removal"), I experimented with reducing these cases down into two steps: compute the new val and then set the location.

Having back-to-back relocation case blocks wasn't ideal, but it does reduce code a bit:

For step 1:
- combine the relative relocation assignment
- consider !apply to be val of 0, drop the zero variable

For step 2:
- drop the if (apply) conditional, just write the new val

For at least this arch, I think it came out OK. Summarized here for reference:

/* Calculate value (or zero if clearing) */

if (apply) {
        val = sym->st_value + rel[i].r_addend;

        switch (ELF64_R_TYPE(rel[i].r_info)) {
        case R_X86_64_PC32:
        case R_X86_64_PLT32:
        case R_X86_64_PC64:
                val -= (u64)loc;
                break;
        }
} else {
        val = 0ULL;
}

/* Apply/clear relocation value */
switch (ELF64_R_TYPE(rel[i].r_info)) {
case R_X86_64_NONE:

        break;
case R_X86_64_64:
        if (apply && *(u64 *)loc != 0)
                goto invalid_relocation;
        write(loc, &val, 8);
        break;
case R_X86_64_32:
        if (apply && *(u32 *)loc != 0)
                goto invalid_relocation;
        write(loc, &val, 4);
        if (val != *(u32 *)loc)
                goto overflow;
        break;

[ ... etc ... ]

That said, things got hairy really fast when I tried applying a similar pattern to ppc64le code, so maybe this model wouldn't help other arches so much. (I haven't looked at s390x yet.)

I'm not married to this approach, but thought I'd mention it as it helped me tease apart these long apply_relocation() functions.

Quoted 6 lines

Quoted 6 lines

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

                        break;
                default:
                        pr_err("%s: Unknown rela relocation: %llu\n",

@@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
        return -ENOEXEC;
 }

Quoted 2 lines

Quoted 1 line

I will try to get around to testing s390x sometime soon and report back on how that works out.

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

arch/powerpc/kernel/module_64.c +17 -0

@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+		       const char *strtab,
+		       unsigned int symindex,
+		       unsigned int relsec,
+		       struct module *me)
+{
+	unsigned int i;
+	Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+	Elf64_Sym *sym;
+	unsigned long *location;
+	const char *symname;
+	u32 *instruction;
+
+	pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+		 sechdrs[relsec].sh_info);
+

arch/powerpc/kernel/module_64.c +4 -0

+
+		instruction += 1;
+		/*
+		 * Patch location + 1 back to NOP so the next

arch/powerpc/kernel/module_64.c +0 -0

 #ifdef CONFIG_DYNAMIC_FTRACE
 int module_trampoline_target(struct module *mod, unsigned long addr,
 			     unsigned long *target)

kernel/livepatch/core.c +18 -0

@@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
 	return 0;
 }
 
+static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+				    const char *shstrtab, const char *strtab,
+				    unsigned int symndx, unsigned int secndx,
+				    const char *objname, bool apply)
+{
+	int cnt, ret;
+	char sec_objname[MODULE_NAME_LEN];
+	Elf_Shdr *sec = sechdrs + secndx;
+
+	/*
+	 * 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",

kernel/livepatch/core.c +0 -20

@@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
 			     unsigned int symndx, unsigned int secndx,
 			     const char *objname)
 {
-	int cnt, ret;
-	char sec_objname[MODULE_NAME_LEN];
-	Elf_Shdr *sec = sechdrs + secndx;
-
-	/*
-	 * 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;
-	}
-
-	if (strcmp(objname ? objname : "vmlinux", sec_objname))
-		return 0;
-

Quoted 44 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

arch/powerpc/kernel/module_64.c +17 -0

@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
      return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                    const char *strtab,
+                    unsigned int symindex,
+                    unsigned int relsec,
+                    struct module *me)
+{
+     unsigned int i;
+     Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+     Elf64_Sym *sym;
+     unsigned long *location;
+     const char *symname;
+     u32 *instruction;
+
+     pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+              sechdrs[relsec].sh_info);
+

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 768

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+     for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+             location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+                     + rela[i].r_offset;
+             sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                     + ELF64_R_SYM(rela[i].r_info);
+             symname = me->core_kallsyms.strtab
+                     + sym->st_name;
+
+             if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                     continue;

Is it OK to continue?

IMHO, we should at least warn here. It means that the special elf section contains a relocation that we are not able to clear. It will most likely blow up when we try to load the livepatched module again.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 775

Quoted 7 lines

arch/powerpc/kernel/module_64.c +7 -0

+             /*
+              * reverse the operations in apply_relocate_add() for case
+              * R_PPC_REL24.
+              */
+             if (sym->st_shndx != SHN_UNDEF &&
+                 sym->st_shndx != SHN_LIVEPATCH)
+                     continue;

Same here. IMHO, we should warn when the section contains something that we are not able to clear.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 779

Quoted 3 lines

arch/powerpc/kernel/module_64.c +3 -0

+             /* skip mprofile and ftrace calls, same as restore_r2() */
+             if (is_mprofile_ftrace_call(symname))
+                     continue;

Is this correct? restore_r2() returns "1" in this case. As a result apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we should print a warning and return as well.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 784

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+             instruction = (u32 *)location;
+             /* skip sibling call, same as restore_r2() */
+             if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+                     continue;

Same here. restore_r2() returns '1' in this case...

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+
+             instruction += 1;
+             /*
+              * Patch location + 1 back to NOP so the next

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+              * apply_relocate_add() call (reload the module) will not
+              * fail the sanity check in restore_r2():
+              *
+              *         if (*instruction != PPC_RAW_NOP()) {
+              *             pr_err(...);
+              *             return 0;
+              *         }
+              */
+             patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
+     }

This seems incomplete. The above code reverts patch_instruction() called from restore_r2(). But there is another patch_instruction() called in apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this as well.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 2 lines

arch/powerpc/kernel/module_64.c +2 -0

+}
+#endif

IMHO, this approach is really bad. The function is not maintainable. It will be very hard to keep it in sync with apply_relocate_add(). And all the mistakes are just a proof.

IMHO, the only sane way is to avoid the code duplication.

Quoted 110 lines · Petr Mladek

Quoted 44 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

arch/powerpc/kernel/module_64.c +17 -0

@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
      return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                    const char *strtab,
+                    unsigned int symindex,
+                    unsigned int relsec,
+                    struct module *me)
+{
+     unsigned int i;
+     Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+     Elf64_Sym *sym;
+     unsigned long *location;
+     const char *symname;
+     u32 *instruction;
+
+     pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+              sechdrs[relsec].sh_info);
+

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 768

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+     for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+             location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+                     + rela[i].r_offset;
+             sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                     + ELF64_R_SYM(rela[i].r_info);
+             symname = me->core_kallsyms.strtab
+                     + sym->st_name;
+
+             if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                     continue;

Is it OK to continue?

IMHO, we should at least warn here. It means that the special elf section contains a relocation that we are not able to clear. It will most likely blow up when we try to load the livepatched module again.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 775

Quoted 7 lines

arch/powerpc/kernel/module_64.c +7 -0

+             /*
+              * reverse the operations in apply_relocate_add() for case
+              * R_PPC_REL24.
+              */
+             if (sym->st_shndx != SHN_UNDEF &&
+                 sym->st_shndx != SHN_LIVEPATCH)
+                     continue;

Same here. IMHO, we should warn when the section contains something that we are not able to clear.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 779

Quoted 3 lines

arch/powerpc/kernel/module_64.c +3 -0

+             /* skip mprofile and ftrace calls, same as restore_r2() */
+             if (is_mprofile_ftrace_call(symname))
+                     continue;

Is this correct? restore_r2() returns "1" in this case. As a result apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we should print a warning and return as well.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 784

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+             instruction = (u32 *)location;
+             /* skip sibling call, same as restore_r2() */
+             if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+                     continue;

Same here. restore_r2() returns '1' in this case...

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+
+             instruction += 1;
+             /*
+              * Patch location + 1 back to NOP so the next

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+              * apply_relocate_add() call (reload the module) will not
+              * fail the sanity check in restore_r2():
+              *
+              *         if (*instruction != PPC_RAW_NOP()) {
+              *             pr_err(...);
+              *             return 0;
+              *         }
+              */
+             patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
+     }

This seems incomplete. The above code reverts patch_instruction() called from restore_r2(). But there is another patch_instruction() called in apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this as well.

Quoted 6 lines

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 2 lines

arch/powerpc/kernel/module_64.c +2 -0

+}
+#endif

IMHO, this approach is really bad. The function is not maintainable. It will be very hard to keep it in sync with apply_relocate_add(). And all the mistakes are just a proof.

I don't really think the above are mistakes. This should be the same as the version that passed Joe's tests. (I didn't test it myself).

Quoted 1 line

Quoted 1 line

IMHO, the only sane way is to avoid the code duplication.

I think this falls back to the question that do we want clear_relocate_add() to
1) undo everything by apply_relocate_add();
or
2) make sure the next apply_relocate_add() succeeds.

Current version does 2). If we want to share a lot of code between apply_ and clear_, we need to go with 1). Do we want something like:

                /* `Everything is relative'. */
                value = sym->st_value + rela[i].r_addend;
                if (!apply)
                        value = 0;

                switch (ELF64_R_TYPE(rela[i].r_info)) {
                case R_PPC64_ADDR32:
                        /* Simply set it */
                        *(u32 *)location = value;
                        break;

                case R_PPC64_ADDR64:
                        /* Simply set it */
                        *(unsigned long *)location = value;
                        break;

                case R_PPC64_TOC:
                       value = apply ? my_r2(sechdrs, me) : 0;
                        *(unsigned long *)location = value;
                        break;

... (a lot more).

Actually, since R_PPC64_ADDR32 etc. don't cause the next apply_ to fail, we can make clear_ to the same thing as apply_ (write the same value again).

These approaches don't look better to me. But I am ok with any of them. Please just let me know which one is most preferable:

a. current version;
b. clear_ undo everything of apply_ (the sample code
above)
c. clear_ undo R_PPC_REL24, but _redo_ everything
of apply_ for other ELF64_R_TYPEs. (should be
clearer code than option b).

btw: undo the follow logic for R_PPC_REL24 alone is not really easy (for me)

                case R_PPC_REL24:
                        /* FIXME: Handle weak symbols here --RR */
                        if (sym->st_shndx == SHN_UNDEF ||
                            sym->st_shndx == SHN_LIVEPATCH) {
                                /* External: go via stub */
                                value = stub_for_addr(sechdrs, value, me,
                                                strtab + sym->st_name);
                                if (!value)
                                        return -ENOENT;
                                if (!restore_r2(strtab + sym->st_name,
                                                        (u32
*)location + 1, me))
                                        return -ENOEXEC;
                        } else
                                value += local_entry_offset(sym);

                        /* Convert value to relative */
                        value -= (unsigned long)location;
                        if (value + 0x2000000 > 0x3ffffff || (value & 3) != 0){
                                pr_err("%s: REL24 %li out of range!\n",
                                       me->name, (long int)value);
                                return -ENOEXEC;
                        }

                        /* Only replace bits 2 through 26 */
                        value = (*(uint32_t *)location & ~PPC_LI_MASK)

| PPC_LI(value);

                        if (patch_instruction((u32 *)location, ppc_inst(value)))
                                return -EFAULT;

                        break;

Quoted 122 lines · Song Liu

Quoted 110 lines · Petr Mladek

Quoted 44 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

arch/powerpc/kernel/module_64.c +17 -0

@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
      return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                    const char *strtab,
+                    unsigned int symindex,
+                    unsigned int relsec,
+                    struct module *me)
+{
+     unsigned int i;
+     Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+     Elf64_Sym *sym;
+     unsigned long *location;
+     const char *symname;
+     u32 *instruction;
+
+     pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+              sechdrs[relsec].sh_info);
+

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 768

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+     for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+             location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+                     + rela[i].r_offset;
+             sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                     + ELF64_R_SYM(rela[i].r_info);
+             symname = me->core_kallsyms.strtab
+                     + sym->st_name;
+
+             if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                     continue;

Is it OK to continue?

IMHO, we should at least warn here. It means that the special elf section contains a relocation that we are not able to clear. It will most likely blow up when we try to load the livepatched module again.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 775

Quoted 7 lines

arch/powerpc/kernel/module_64.c +7 -0

+             /*
+              * reverse the operations in apply_relocate_add() for case
+              * R_PPC_REL24.
+              */
+             if (sym->st_shndx != SHN_UNDEF &&
+                 sym->st_shndx != SHN_LIVEPATCH)
+                     continue;

Same here. IMHO, we should warn when the section contains something that we are not able to clear.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 779

Quoted 3 lines

arch/powerpc/kernel/module_64.c +3 -0

+             /* skip mprofile and ftrace calls, same as restore_r2() */
+             if (is_mprofile_ftrace_call(symname))
+                     continue;

Is this correct? restore_r2() returns "1" in this case. As a result apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we should print a warning and return as well.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 784

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+             instruction = (u32 *)location;
+             /* skip sibling call, same as restore_r2() */
+             if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+                     continue;

Same here. restore_r2() returns '1' in this case...

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+
+             instruction += 1;
+             /*
+              * Patch location + 1 back to NOP so the next

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+              * apply_relocate_add() call (reload the module) will not
+              * fail the sanity check in restore_r2():
+              *
+              *         if (*instruction != PPC_RAW_NOP()) {
+              *             pr_err(...);
+              *             return 0;
+              *         }
+              */
+             patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
+     }

This seems incomplete. The above code reverts patch_instruction() called from restore_r2(). But there is another patch_instruction() called in apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this as well.

Quoted 6 lines

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 2 lines

arch/powerpc/kernel/module_64.c +2 -0

+}
+#endif

IMHO, this approach is really bad. The function is not maintainable. It will be very hard to keep it in sync with apply_relocate_add(). And all the mistakes are just a proof.

I don't really think the above are mistakes. This should be the same as the version that passed Joe's tests. (I didn't test it myself).

Quoted 1 line

Quoted 8 lines

Quoted 1 line

IMHO, the only sane way is to avoid the code duplication.

I think this falls back to the question that do we want clear_relocate_add() to
1) undo everything by apply_relocate_add();
or
2) make sure the next apply_relocate_add() succeeds.

This is a really good question and I think relates to your follow up question to my earlier reply, "What's the failure like if we don't handle R_PPC64_ADDR64 and R_PPC64_REL64?"

If the code only needs to accomplish (2), then the incoming patch simply overwrites old relocation values. If we prefer (1), then needs to do the full reversal on unload.

Stepping back, this feature is definitely foot-gun capable. Kpatch-build would expect that klp-relocations would only ever be needed in code that will patch the very same module that provides the relocation destination -- that is, it was never intended to reference through one of these klp-relocations unless it resolved to a live module.

On the other hand, when writing the selftests, verifying against NULL
[1] provided 1) a quick sanity check that something was "cleared" and 2) protected the machine against said foot-gun.

[1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c

Quoted 11 lines

These approaches don't look better to me. But I am ok with any of them. Please just let me know which one is most preferable:

a. current version;
b. clear_ undo everything of apply_ (the sample code
above)
c. clear_ undo R_PPC_REL24, but _redo_ everything
of apply_ for other ELF64_R_TYPEs. (should be
clearer code than option b).

This was my attempt at combining and slightly refactoring the power64 version. There is so much going on here I was tempted to split off it into separate value assignment and write functions. Some changes I liked, but I wasn't all too happy with the result. Also, as you mention, completely undoing R_PPC_REL24 is less than trivial... for this arch, there are basically three major tasks:

1) calculate the new value, including range checking
2) special constructs created by restore_r2 / create_stub
3) writing out the value

and many cases are similar, but subtly different enough to avoid easy code consolidation.

static int write_relocate_add(Elf64_Shdr *sechdrs,
const char *strtab, unsigned int symindex, unsigned int relsec, struct module *me,
bool apply)

{
        unsigned int i;
        Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
        Elf64_Sym *sym;
        unsigned long *location;
        unsigned long value;

Quoted 9 lines · Joe Lawrence

Stepping back, this feature is definitely foot-gun capable. Kpatch-build would expect that klp-relocations would only ever be needed in code that will patch the very same module that provides the relocation destination -- that is, it was never intended to reference through one of these klp-relocations unless it resolved to a live module.

Quoted 5 lines

On the other hand, when writing the selftests, verifying against NULL
[1] provided 1) a quick sanity check that something was "cleared" and 2) protected the machine against said foot-gun.

[1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c

I don't quite follow the foot-gun here. What's the failure mode?

[...]

Quoted 19 lines

Quoted 11 lines

These approaches don't look better to me. But I am ok with any of them. Please just let me know which one is most preferable:

a. current version;
b. clear_ undo everything of apply_ (the sample code
above)
c. clear_ undo R_PPC_REL24, but _redo_ everything
of apply_ for other ELF64_R_TYPEs. (should be
clearer code than option b).

This was my attempt at combining and slightly refactoring the power64 version. There is so much going on here I was tempted to split off it into separate value assignment and write functions. Some changes I liked, but I wasn't all too happy with the result. Also, as you mention, completely undoing R_PPC_REL24 is less than trivial... for this arch, there are basically three major tasks:

Quoted 6 lines

1) calculate the new value, including range checking
2) special constructs created by restore_r2 / create_stub
3) writing out the value

and many cases are similar, but subtly different enough to avoid easy code consolidation.

Thanks for exploring this direction. I guess this part won't be perfect anyway.

PS: While we discuss a solution for ppc64, how about we ship the fix for other archs first? I think there are only a few small things to be addressed.

Quoted 10 lines · Song Liu

Quoted 9 lines · Joe Lawrence

Stepping back, this feature is definitely foot-gun capable. Kpatch-build would expect that klp-relocations would only ever be needed in code that will patch the very same module that provides the relocation destination -- that is, it was never intended to reference through one of these klp-relocations unless it resolved to a live module.

Quoted 8 lines

Quoted 5 lines

On the other hand, when writing the selftests, verifying against NULL
[1] provided 1) a quick sanity check that something was "cleared" and 2) protected the machine against said foot-gun.

[1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c

I don't quite follow the foot-gun here. What's the failure mode?

Kpatch-build, for better or worse, hides the potential problem. A typical kpatch scenario would be:

1. A patch modifies module foo's function bar(), which references symbols local to module foo

2. Kpatch-build creates a livepatch .ko with klp-relocations in the modified bar() to foo's symbols

3. When loaded, modified bar() code that references through its klp-relocations to module foo will only ever be active when foo is loaded, i.e. when the original bar() redirects to the livepatch version.

However, writing source-based livepatches (like the kselftests) offers a lot more freedom. There is no implicit guarantee from (3) that the module is loaded. One could reference klp-relocations from anywhere in the livepatch module.

For example, in my test_klp_convert1.c test case, I have a livepatch module with a sysfs interface (print_debug_set()) that allows the test bash script to force the module to references through its klp-relocations (print_static_strings()):

...
static void print_string(const char *label, const char *str)

{
        if (str)
                pr_info("%s: %s\n", label, str);
}
...
static noinline void print_static_strings(void)
{
        print_string("klp_string.12345", klp_string_a);
        print_string("klp_string.67890", klp_string_b);
}

/* provide a sysfs handle to invoke debug functions */
static int print_debug;
static int print_debug_set(const char *val, const struct kernel_param *kp)

{
        print_static_strings();

        return 0;
}

static const struct kernel_param_ops print_debug_ops = {
.set = print_debug_set,
.get = param_get_int,
};

When I first wrote test_klp_convert1.c, I did not have wrappers like print_string(), I simply referenced the symbols directly and send them to pr_info as "%s" string formatting options.

You can probably see where this is going when I unloaded the module that provided klp_string_a, klp_string_b, etc. and invoked the sysfs handle. The stale relocation values point to ... somewhere we shouldn't try to reference any more.

Perhaps I'm too paranoid about that possibility, but by actually clearing the values in the relocations on module removal, one could check them and try to guard against dangling pointer (dangling relocation?) references.

Quoted 10 lines · Song Liu

Quoted 9 lines · Joe Lawrence

Stepping back, this feature is definitely foot-gun capable. Kpatch-build would expect that klp-relocations would only ever be needed in code that will patch the very same module that provides the relocation destination -- that is, it was never intended to reference through one of these klp-relocations unless it resolved to a live module.

Quoted 8 lines

Quoted 5 lines

On the other hand, when writing the selftests, verifying against NULL
[1] provided 1) a quick sanity check that something was "cleared" and 2) protected the machine against said foot-gun.

[1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c

I don't quite follow the foot-gun here. What's the failure mode?

Kpatch-build, for better or worse, hides the potential problem. A typical kpatch scenario would be:

1. A patch modifies module foo's function bar(), which references symbols local to module foo

2. Kpatch-build creates a livepatch .ko with klp-relocations in the modified bar() to foo's symbols

3. When loaded, modified bar() code that references through its klp-relocations to module foo will only ever be active when foo is loaded, i.e. when the original bar() redirects to the livepatch version.

However, writing source-based livepatches (like the kselftests) offers a lot more freedom. There is no implicit guarantee from (3) that the module is loaded. One could reference klp-relocations from anywhere in the livepatch module.

Quoted 31 lines

[...]

Quoted 19 lines

Quoted 11 lines

These approaches don't look better to me. But I am ok with any of them. Please just let me know which one is most preferable:

a. current version;
b. clear_ undo everything of apply_ (the sample code
above)
c. clear_ undo R_PPC_REL24, but _redo_ everything
of apply_ for other ELF64_R_TYPEs. (should be
clearer code than option b).

This was my attempt at combining and slightly refactoring the power64 version. There is so much going on here I was tempted to split off it into separate value assignment and write functions. Some changes I liked, but I wasn't all too happy with the result. Also, as you mention, completely undoing R_PPC_REL24 is less than trivial... for this arch, there are basically three major tasks:

Quoted 6 lines

1) calculate the new value, including range checking
2) special constructs created by restore_r2 / create_stub
3) writing out the value

and many cases are similar, but subtly different enough to avoid easy code consolidation.

Thanks for exploring this direction. I guess this part won't be perfect anyway.

Quoted 4 lines

PS: While we discuss a solution for ppc64, how about we ship the fix for other archs first? I think there are only a few small things to be addressed.

Yeah, the x86_64 version looks a lot simpler and closer to being done. Though I believe that Petr would prefer a complete solution, but I'll let him speak to that.

Hi

Quoted 33 lines · Joe Lawrence

Quoted 10 lines · Song Liu

Quoted 9 lines · Joe Lawrence

Stepping back, this feature is definitely foot-gun capable. Kpatch-build would expect that klp-relocations would only ever be needed in code that will patch the very same module that provides the relocation destination -- that is, it was never intended to reference through one of these klp-relocations unless it resolved to a live module.

Quoted 8 lines

Quoted 5 lines

On the other hand, when writing the selftests, verifying against NULL
[1] provided 1) a quick sanity check that something was "cleared" and 2) protected the machine against said foot-gun.

[1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c

I don't quite follow the foot-gun here. What's the failure mode?

Kpatch-build, for better or worse, hides the potential problem. A typical kpatch scenario would be:

1. A patch modifies module foo's function bar(), which references symbols local to module foo

2. Kpatch-build creates a livepatch .ko with klp-relocations in the modified bar() to foo's symbols

3. When loaded, modified bar() code that references through its klp-relocations to module foo will only ever be active when foo is loaded, i.e. when the original bar() redirects to the livepatch version.

Quoted 4 lines

However, writing source-based livepatches (like the kselftests) offers a lot more freedom. There is no implicit guarantee from (3) that the module is loaded. One could reference klp-relocations from anywhere in the livepatch module.

Yes, on the other hand the approach you describe above seems to be the only reasonable one in my opinion. The rest might be considered a bug. Foot-gun as you say. I am not sure if we can do anything about it.

Quoted 31 lines

Quoted 31 lines

[...]

Quoted 19 lines

Quoted 11 lines

These approaches don't look better to me. But I am ok with any of them. Please just let me know which one is most preferable:

a. current version;
b. clear_ undo everything of apply_ (the sample code
above)
c. clear_ undo R_PPC_REL24, but _redo_ everything
of apply_ for other ELF64_R_TYPEs. (should be
clearer code than option b).

This was my attempt at combining and slightly refactoring the power64 version. There is so much going on here I was tempted to split off it into separate value assignment and write functions. Some changes I liked, but I wasn't all too happy with the result. Also, as you mention, completely undoing R_PPC_REL24 is less than trivial... for this arch, there are basically three major tasks:

Quoted 6 lines

1) calculate the new value, including range checking
2) special constructs created by restore_r2 / create_stub
3) writing out the value

and many cases are similar, but subtly different enough to avoid easy code consolidation.

Thanks for exploring this direction. I guess this part won't be perfect anyway.

Quoted 8 lines

Quoted 4 lines

PS: While we discuss a solution for ppc64, how about we ship the fix for other archs first? I think there are only a few small things to be addressed.

Yeah, the x86_64 version looks a lot simpler and closer to being done. Though I believe that Petr would prefer a complete solution, but I'll let him speak to that.

I cannot speak for Petr, but I think it might be easier to split it given the situation. Then we can involve arch maintainers for ppc64le because they might have a preference with respect to a, b, c options above.

Petr, what do you think?

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

PS: While we discuss a solution for ppc64, how about we ship the fix for other archs first? I think there are only a few small things to be addressed.

Yeah, the x86_64 version looks a lot simpler and closer to being done. Though I believe that Petr would prefer a complete solution, but I'll let him speak to that.

Quoted 5 lines

I cannot speak for Petr, but I think it might be easier to split it given the situation. Then we can involve arch maintainers for ppc64le because they might have a preference with respect to a, b, c options above.

Petr, what do you think?

I am fine with solving each architecture in a separate patch or patchset. It would make it easier, especially, for arch maintainers.

Quoted 110 lines · Petr Mladek

Quoted 44 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

arch/powerpc/kernel/module_64.c +17 -0

@@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
      return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                    const char *strtab,
+                    unsigned int symindex,
+                    unsigned int relsec,
+                    struct module *me)
+{
+     unsigned int i;
+     Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
+     Elf64_Sym *sym;
+     unsigned long *location;
+     const char *symname;
+     u32 *instruction;
+
+     pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
+              sechdrs[relsec].sh_info);
+

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 768

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+     for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
+             location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+                     + rela[i].r_offset;
+             sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+                     + ELF64_R_SYM(rela[i].r_info);
+             symname = me->core_kallsyms.strtab
+                     + sym->st_name;
+
+             if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
+                     continue;

Is it OK to continue?

IMHO, we should at least warn here. It means that the special elf section contains a relocation that we are not able to clear. It will most likely blow up when we try to load the livepatched module again.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 775

Quoted 7 lines

arch/powerpc/kernel/module_64.c +7 -0

+             /*
+              * reverse the operations in apply_relocate_add() for case
+              * R_PPC_REL24.
+              */
+             if (sym->st_shndx != SHN_UNDEF &&
+                 sym->st_shndx != SHN_LIVEPATCH)
+                     continue;

Same here. IMHO, we should warn when the section contains something that we are not able to clear.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 779

Quoted 3 lines

arch/powerpc/kernel/module_64.c +3 -0

+             /* skip mprofile and ftrace calls, same as restore_r2() */
+             if (is_mprofile_ftrace_call(symname))
+                     continue;

Is this correct? restore_r2() returns "1" in this case. As a result apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we should print a warning and return as well.

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 784

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+             instruction = (u32 *)location;
+             /* skip sibling call, same as restore_r2() */
+             if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+                     continue;

Same here. restore_r2() returns '1' in this case...

Quoted 4 lines

arch/powerpc/kernel/module_64.c +4 -0

+
+             instruction += 1;
+             /*
+              * Patch location + 1 back to NOP so the next

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 10 lines

arch/powerpc/kernel/module_64.c +10 -0

+              * apply_relocate_add() call (reload the module) will not
+              * fail the sanity check in restore_r2():
+              *
+              *         if (*instruction != PPC_RAW_NOP()) {
+              *             pr_err(...);
+              *             return 0;
+              *         }
+              */
+             patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
+     }

This seems incomplete. The above code reverts patch_instruction() called from restore_r2(). But there is another patch_instruction() called in apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this as well.

Quoted 6 lines

arch/powerpc/kernel/module_64.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 798

Quoted 2 lines

arch/powerpc/kernel/module_64.c +2 -0

+}
+#endif

IMHO, this approach is really bad. The function is not maintainable. It will be very hard to keep it in sync with apply_relocate_add(). And all the mistakes are just a proof.

I don't really think the above are mistakes. This should be the same as the version that passed Joe's tests. (I didn't test it myself).

Quoted 2 lines

IMHO, the only sane way is to avoid the code duplication.

I think this falls back to the question that do we want clear_relocate_add() to
1) undo everything by apply_relocate_add();
or
2) make sure the next apply_relocate_add() succeeds.

The ideal solution would be to add checks into apply_relocated_add(). It would make it more robust. In that case, clear_relocated_add() would need to clear everything.

But this is not the case on powerpc and s390 at the moment. In this case, I suggest to clear only relocations that are checked in apply_relocated_add().

But it should be done without duplicating the code.

It would actually make sense to compute the value that was used in apply_relocated_add() and check that we are clearing the value. If we try to clear some other value than we probably do something wrong.

This might actually be a solution. We could compute the value in both situations. Then we could have a common function for writing.

This write function would check that it replaces zero with the value in apply_relocate_add() and that it replaces the value with zero in clear_relocate_add().

Btw: I am confused with this one:

                case R_PPC64_REL16_HA:
                        /* Subtract location pointer */
                        value -= (unsigned long)location;
                        value = ((value + 0x8000) >> 16);
                        *((uint16_t *) location)
                                = (*((uint16_t *) location) & ~0xffff)
                                | (value & 0xffff);
                        break;

(*((uint16_t *) location) & ~0xffff) should always be zero, no?

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

He also proposed three different solutions. We could remove the error check in apply_relocate_add() introduced by commit eda9cec4c9a1 ("x86/module: Detect and skip invalid relocations"). However the check is useful for detecting corrupted modules.

We could also deny the patched modules to be removed. If it proved to be a major drawback for users, we could still implement a different approach. The solution would also complicate the existing code a lot.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

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

@@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
 		case R_X86_64_NONE:
 			break;