livepatch: Clear relocation targets on a module removal

PATCH Superseded Song Liu 2 participants 4 messages Aug 1, 2022 – Aug 30, 2022

Subsystems (5)

x86 PowerPC s390 Module support +1 more subsystems

lore

Share & tools

lore

Song Liu

author maintainer

2022-08-01 21:22 UTC #0 lore raw

PATCH v4 1/1 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.

Reported-by Josh Poimboeuf <[email protected]>

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

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

---

NOTE: powerpc code has not be tested.

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_64.c |  49 +++++++++++++++
 arch/s390/kernel/module.c       |   8 +++
 arch/x86/kernel/module.c        | 102 +++++++++++++++++++++++---------
 include/linux/moduleloader.h    |   7 +++
 kernel/livepatch/core.c         |  41 ++++++++++++-
 5 files changed, 179 insertions(+), 28 deletions(-)

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

index 7e45dc98df8a..1834dffc6795 100644
@@ -739,6 +739,55 @@ 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;
+
+		instruction = (u32 *)location;
+		if (is_mprofile_ftrace_call(symname))
+			continue;
+
+		if (!instr_is_relative_link_branch(*instruction))
+			continue;
+
+		instruction += 1;
+		*instruction = PPC_INST_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 +75 ~~-27~~

147 lines not on this page · raw message

include/linux/moduleloader.h +7 -0

index 9e09d11ffe5b..d22b36b84b4b 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(Elf64_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 +40 -1

53 lines not on this page · raw message

Signature · 1 line

2.30.2

Joe Lawrence

maintainer

2022-08-05 20:58 UTC #1 lore raw

Quoted 36 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.

Quoted 8 lines

Reported-by Josh Poimboeuf <[email protected]>
Signed-off-by Miroslav Benes <[email protected]>
Signed-off-by Song Liu <[email protected]>
---
NOTE: powerpc code has not be tested.

Hi Song,

I just want to provide a quick check in on this patch...

First -- what tree / commit should this be based on? When I add this patch on top of a v5.19 based tree, I see:

arch/powerpc/kernel/module_64.c: In function ‘clear_relocate_add’:
arch/powerpc/kernel/module_64.c:781:52: error: incompatible type for argument 1 of ‘instr_is_relative_link_branch’

  781 |                 if (!instr_is_relative_link_branch(*instruction))
      |                                                    ^~~~~~~~~~~~
      |                                                    |
      |                                                    u32 {aka unsigned int}
In file included from arch/powerpc/kernel/module_64.c:20:
./arch/powerpc/include/asm/code-patching.h:122:46: note: expected ‘ppc_inst_t’ but argument is of type ‘u32’ {aka ‘unsigned int’}
  122 | int instr_is_relative_link_branch(ppc_inst_t instr);
      |                                   ~~~~~~~~~~~^~~~~
arch/powerpc/kernel/module_64.c:785:32: error: ‘PPC_INST_NOP’ undeclared (first use in this function); did you mean ‘PPC_INST_COPY’?
  785 |                 *instruction = PPC_INST_NOP;
      |                                ^~~~~~~~~~~~
      |                                PPC_INST_COPY

arch/powerpc/kernel/module_64.c:785:32: note: each undeclared identifier is reported only once for each function it appears in make[2]: *** [scripts/Makefile.build:249: arch/powerpc/kernel/module_64.o] Error 1
make[1]: *** [scripts/Makefile.build:466: arch/powerpc/kernel] Error 2
make: *** [Makefile:1849: arch/powerpc] Error 2

Second, I rebased the klp-convert-tree on top of v5.19 here:
https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel

and I can confirm that at least the x86_64 livepatching selftests (including the klp-relocation tests added by this tree) do pass. I haven't had a chance to try writing new tests to verify this specific patch, but I'll take a look next week.

Regards,

--
Joe

Song Liu

author maintainer

2022-08-05 21:33 UTC #2 lore raw

Quoted 68 lines · Joe Lawrence

Quoted 36 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.
Quoted 8 lines
Reported-by Josh Poimboeuf <[email protected]>
Signed-off-by Miroslav Benes <[email protected]>
Signed-off-by Song Liu <[email protected]>
---
NOTE: powerpc code has not be tested.
Hi Song,
I just want to provide a quick check in on this patch...
First -- what tree / commit should this be based on? When I add this patch on top of a v5.19 based tree, I see:
arch/powerpc/kernel/module_64.c: In function ‘clear_relocate_add’:
arch/powerpc/kernel/module_64.c:781:52: error: incompatible type for argument 1 of ‘instr_is_relative_link_branch’
  781 |                 if (!instr_is_relative_link_branch(*instruction))
      |                                                    ^~~~~~~~~~~~
      |                                                    |
      |                                                    u32 {aka unsigned int}
In file included from arch/powerpc/kernel/module_64.c:20:
./arch/powerpc/include/asm/code-patching.h:122:46: note: expected ‘ppc_inst_t’ but argument is of type ‘u32’ {aka ‘unsigned int’}
  122 | int instr_is_relative_link_branch(ppc_inst_t instr);
      |                                   ~~~~~~~~~~~^~~~~
arch/powerpc/kernel/module_64.c:785:32: error: ‘PPC_INST_NOP’ undeclared (first use in this function); did you mean ‘PPC_INST_COPY’?
  785 |                 *instruction = PPC_INST_NOP;
      |                                ^~~~~~~~~~~~
      |                                PPC_INST_COPY

Quoted 5 lines

arch/powerpc/kernel/module_64.c:785:32: note: each undeclared identifier is reported only once for each function it appears in make[2]: *** [scripts/Makefile.build:249: arch/powerpc/kernel/module_64.o] Error 1
make[1]: *** [scripts/Makefile.build:466: arch/powerpc/kernel] Error 2
make: *** [Makefile:1849: arch/powerpc] Error 2

I am sorry that I didn't build the PPC code. (I did fix some code, but I guess that's
not enough. ) I was hoping kernel test bot to run build tests on the patch, but I
guess the bot is not following live-patching mail list?

The code was based Linus' tree, probably 5.19-rc7.

Quoted 8 lines

Second, I rebased the klp-convert-tree on top of v5.19 here:
https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel
and I can confirm that at least the x86_64 livepatching selftests (including the klp-relocation tests added by this tree) do pass. I haven't had a chance to try writing new tests to verify this specific patch, but I'll take a look next week.

I also got the selftests pass for another patch. Checking dmesg is a little tricky, btw. I will take a look at klp-convert.

Thanks,
Song

Song Liu

author maintainer

2022-08-30 07:18 UTC #3 lore raw

Quoted 67 lines · Song Liu

Quoted 65 lines · Joe Lawrence
Quoted 36 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.
Quoted 8 lines
Reported-by Josh Poimboeuf <[email protected]>
Signed-off-by Miroslav Benes <[email protected]>
Signed-off-by Song Liu <[email protected]>
---
NOTE: powerpc code has not be tested.
Hi Song,
I just want to provide a quick check in on this patch...
First -- what tree / commit should this be based on? When I add this patch on top of a v5.19 based tree, I see:
arch/powerpc/kernel/module_64.c: In function ‘clear_relocate_add’:
arch/powerpc/kernel/module_64.c:781:52: error: incompatible type for argument 1 of ‘instr_is_relative_link_branch’
  781 |                 if (!instr_is_relative_link_branch(*instruction))
      |                                                    ^~~~~~~~~~~~
      |                                                    |
      |                                                    u32 {aka unsigned int}
In file included from arch/powerpc/kernel/module_64.c:20:
./arch/powerpc/include/asm/code-patching.h:122:46: note: expected ‘ppc_inst_t’ but argument is of type ‘u32’ {aka ‘unsigned int’}
  122 | int instr_is_relative_link_branch(ppc_inst_t instr);
      |                                   ~~~~~~~~~~~^~~~~
arch/powerpc/kernel/module_64.c:785:32: error: ‘PPC_INST_NOP’ undeclared (first use in this function); did you mean ‘PPC_INST_COPY’?

Quoted 8 lines

Quoted 8 lines
  785 |                 *instruction = PPC_INST_NOP;
      |                                ^~~~~~~~~~~~
      |                                PPC_INST_COPY
arch/powerpc/kernel/module_64.c:785:32: note: each undeclared identifier is reported only once for each function it appears in make[2]: *** [scripts/Makefile.build:249: arch/powerpc/kernel/module_64.o] Error 1
make[1]: *** [scripts/Makefile.build:466: arch/powerpc/kernel] Error 2
make: *** [Makefile:1849: arch/powerpc] Error 2

The following should make it build on powerpc64

Shall I send it as v5? (I haven't tested powerpc64 other than cross compile).

Thanks,
Song

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

index 1834dffc6795..6aaf5720070d 100644
@@ -778,11 +778,11 @@ void clear_relocate_add(Elf64_Shdr *sechdrs,
                if (is_mprofile_ftrace_call(symname))
                        continue;
 
-               if (!instr_is_relative_link_branch(*instruction))
+               if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
                        continue;
 
                instruction += 1;
-               *instruction = PPC_INST_NOP;
+               *instruction = PPC_RAW_NOP();
        }
 
 }

Quoted 23 lines

I am sorry that I didn't build the PPC code. (I did fix some code, but I guess that's
not enough. ) I was hoping kernel test bot to run build tests on the patch, but I
guess the bot is not following live-patching mail list?
The code was based Linus' tree, probably 5.19-rc7.
Quoted 8 lines
Second, I rebased the klp-convert-tree on top of v5.19 here:
https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel
and I can confirm that at least the x86_64 livepatching selftests (including the klp-relocation tests added by this tree) do pass. I haven't had a chance to try writing new tests to verify this specific patch, but I'll take a look next week.
I also got the selftests pass for another patch. Checking dmesg is a little tricky, btw. I will take a look at klp-convert.
Thanks,
Song