[PATCH] powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols

Kamalesh Babulal
R_PPC64_REL24 relocation type is used for a function call, where the
function symbol address is resolved and stub code (a.k.a trampoline)
is constructed to jump into the global entry of the function. The
caller is responsible for setting up the TOC of the called function
before branching and NOP is expected after every branch, which gets
replaced with an instruction to restore the callers TOC from the
stack on return.

SHN_LIVEPATCH symbols with R_PPC64_REL24 relocation type might not
adhere to nop instruction after a branch. The reason being called
function was local to the callee and the instruction sequence
generated does not stores and restores the TOC value onto the stack
of the calling function, which is right instead uses the localentry
(function entry + 0x8) to jump into the function and returns without
the need for store/restoring the TOC, as both of the functions were
in the same compilation unit.

When such functions become global because they are livepatched and
every call to the function, re-directs the callee to the patched
version in the module. Every branch from the livepatch module, to
previously local function turns out to jump into the kernel code
but with the code, sequence remains that of the localentry. This
leads to error while trying to access the function with assumption.

insmod: ERROR: could not insert module ./kpatch-meminfo-string.ko: Invalid module format

Jun 13 02:10:47 ubuntu kernel: [   37.774292] module_64: kpatch_meminfo_string: REL24 -1152921504749690968 out of range!

SHN_LIVEPATCH symbols + R_PPC64_REL24 type relocation should be handled
by introducing a new stub code sequence, instead of regular stub code.
Where stub takes care of storing and restoring the Link Register/TOC
onto the stack of the livepatched function and restores them upon
return from the called function.

Signed-off-by: Kamalesh Babulal <[hidden email]>
Cc: Michael Ellerman <[hidden email]>
Cc: Balbir Singh <[hidden email]>
Cc: Josh Poimboeuf <[hidden email]>
Cc: Jessica Yu <[hidden email]>
Cc: [hidden email]
---
 arch/powerpc/kernel/module_64.c | 55 ++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f896..52ded0f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -102,10 +102,16 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
    jump, actually, to reset r2 (TOC+0x8000). */
 struct ppc64_stub_entry
 {
- /* 28 byte jump instruction sequence (7 instructions). We only
- * need 6 instructions on ABIv2 but we always allocate 7 so
- * so we don't have to modify the trampoline load instruction. */
- u32 jump[7];
+ /* 28 byte jump instruction sequence (7 instructions) and only
+ * 6 instructions are needed on ABIv2 to create trampoline.
+ * Trampoline for livepatch symbol needs extra 7 instructions
+ * to save and restore LR, TOC values.
+ *
+ * To accommodate extra instructions required for livepatch
+ * entry, we allocate 15 instructions so we don't have to
+ * modify the trampoline load instruction.
+ */
+ u32 jump[15];
  /* Used by ftrace to identify stubs */
  u32 magic;
  /* Data for the above code */
@@ -131,7 +137,7 @@ static u32 ppc64_stub_insns[] = {
  0x396b0000, /* addi    r11,r11, <low> */
  /* Save current r2 value in magic place on the stack. */
  0xf8410000|R2_STACK_OFFSET, /* std     r2,R2_STACK_OFFSET(r1) */
- 0xe98b0020, /* ld      r12,32(r11) */
+ 0xe98b0040, /* ld      r12,64(r11) */
 #ifdef PPC64_ELF_ABI_v1
  /* Set up new r2 from function descriptor */
  0xe84b0028, /* ld      r2,40(r11) */
@@ -140,6 +146,24 @@ static u32 ppc64_stub_insns[] = {
  0x4e800420 /* bctr */
 };
 
+static u32 ppc64_klp_stub_insns[] = {
+ 0x3d620000,                     /* addis   r11,r2, <high> */
+ 0x396b0000,                     /* addi    r11,r11, <low> */
+ 0x7c0802a6,                     /* mflr    r0 */
+ 0xf8010010,                     /* std     r0,16(r1) */
+ /* Save current r2 value in magic place on the stack. */
+ 0xf8410000|R2_STACK_OFFSET,     /* std     r2,R2_STACK_OFFSET(r1) */
+ 0xe98b0040, /* ld   r12,64(r11) */
+ 0xf821ffe1,                     /* stdu    r1,-32(r1) */
+ 0x7d8903a6,                     /* mtctr   r12 */
+ 0x4e800421,                     /* bctrl */
+ 0x38210020,                     /* addi    r1,r1,32 */
+ 0xe8010010,                     /* ld      r0,16(r1) */
+ 0xe8410000|R2_STACK_OFFSET,     /* ld      r2,R2_STACK_OFFSET(r1) */
+ 0x7c0803a6,                     /* mtlr    r0 */
+ 0x4e800020                      /* blr */
+};
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 int module_trampoline_target(struct module *mod, unsigned long addr,
      unsigned long *target)
@@ -392,11 +416,16 @@ static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module *me)
 static inline int create_stub(const Elf64_Shdr *sechdrs,
       struct ppc64_stub_entry *entry,
       unsigned long addr,
-      struct module *me)
+      struct module *me, int klp_symbol)
 {
  long reladdr;
 
- memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+ memset(entry, 0, sizeof(*entry));
+ if (klp_symbol)
+ memcpy(entry->jump, ppc64_klp_stub_insns,
+ sizeof(ppc64_klp_stub_insns));
+ else
+ memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
 
  /* Stub uses address relative to r2. */
  reladdr = (unsigned long)entry - my_r2(sechdrs, me);
@@ -419,7 +448,7 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
    stub to set up the TOC ptr (r2) for the function. */
 static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
    unsigned long addr,
-   struct module *me)
+   struct module *me, int klp_symbol)
 {
  struct ppc64_stub_entry *stubs;
  unsigned int i, num_stubs;
@@ -435,7 +464,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
  return (unsigned long)&stubs[i];
  }
 
- if (!create_stub(sechdrs, &stubs[i], addr, me))
+ if (!create_stub(sechdrs, &stubs[i], addr, me, klp_symbol))
  return 0;
 
  return (unsigned long)&stubs[i];
@@ -615,13 +644,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
  /* FIXME: Handle weak symbols here --RR */
  if (sym->st_shndx == SHN_UNDEF) {
  /* External: go via stub */
- value = stub_for_addr(sechdrs, value, me);
+ value = stub_for_addr(sechdrs, value, me, 0);
  if (!value)
  return -ENOENT;
  if (!restore_r2((u32 *)location + 1, me))
  return -ENOEXEC;
 
  squash_toc_save_inst(strtab + sym->st_name, value);
+ } else if (sym->st_shndx == SHN_LIVEPATCH) {
+ value = stub_for_addr(sechdrs, value, me, 1);
+ if (!value)
+ return -ENOENT;
  } else
  value += local_entry_offset(sym);
 
@@ -775,7 +808,7 @@ static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module
 #else
 static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
 {
- return stub_for_addr(sechdrs, (unsigned long)ftrace_caller, me);
+ return stub_for_addr(sechdrs, (unsigned long)ftrace_caller, me, 0);
 }
 #endif
 
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols

Kamalesh Babulal
Hi Michael,

Any thoughts/suggestions on the approach ?


On Tuesday 13 June 2017 12:20 PM, Kamalesh Babulal wrote:

> R_PPC64_REL24 relocation type is used for a function call, where the
> function symbol address is resolved and stub code (a.k.a trampoline)
> is constructed to jump into the global entry of the function. The
> caller is responsible for setting up the TOC of the called function
> before branching and NOP is expected after every branch, which gets
> replaced with an instruction to restore the callers TOC from the
> stack on return.
>
> SHN_LIVEPATCH symbols with R_PPC64_REL24 relocation type might not
> adhere to nop instruction after a branch. The reason being called
> function was local to the callee and the instruction sequence
> generated does not stores and restores the TOC value onto the stack
> of the calling function, which is right instead uses the localentry
> (function entry + 0x8) to jump into the function and returns without
> the need for store/restoring the TOC, as both of the functions were
> in the same compilation unit.
>
> When such functions become global because they are livepatched and
> every call to the function, re-directs the callee to the patched
> version in the module. Every branch from the livepatch module, to
> previously local function turns out to jump into the kernel code
> but with the code, sequence remains that of the localentry. This
> leads to error while trying to access the function with assumption.
>
> insmod: ERROR: could not insert module ./kpatch-meminfo-string.ko: Invalid module format
>
> Jun 13 02:10:47 ubuntu kernel: [   37.774292] module_64: kpatch_meminfo_string: REL24 -1152921504749690968 out of range!
>
> SHN_LIVEPATCH symbols + R_PPC64_REL24 type relocation should be handled
> by introducing a new stub code sequence, instead of regular stub code.
> Where stub takes care of storing and restoring the Link Register/TOC
> onto the stack of the livepatched function and restores them upon
> return from the called function.
>
> Signed-off-by: Kamalesh Babulal <[hidden email]>
> Cc: Michael Ellerman <[hidden email]>
> Cc: Balbir Singh <[hidden email]>
> Cc: Josh Poimboeuf <[hidden email]>
> Cc: Jessica Yu <[hidden email]>
> Cc: [hidden email]
> ---
>  arch/powerpc/kernel/module_64.c | 55 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 0b0f896..52ded0f 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -102,10 +102,16 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
>     jump, actually, to reset r2 (TOC+0x8000). */
>  struct ppc64_stub_entry
>  {
> - /* 28 byte jump instruction sequence (7 instructions). We only
> - * need 6 instructions on ABIv2 but we always allocate 7 so
> - * so we don't have to modify the trampoline load instruction. */
> - u32 jump[7];
> + /* 28 byte jump instruction sequence (7 instructions) and only
> + * 6 instructions are needed on ABIv2 to create trampoline.
> + * Trampoline for livepatch symbol needs extra 7 instructions
> + * to save and restore LR, TOC values.
> + *
> + * To accommodate extra instructions required for livepatch
> + * entry, we allocate 15 instructions so we don't have to
> + * modify the trampoline load instruction.
> + */
> + u32 jump[15];
>   /* Used by ftrace to identify stubs */
>   u32 magic;
>   /* Data for the above code */
> @@ -131,7 +137,7 @@ static u32 ppc64_stub_insns[] = {
>   0x396b0000, /* addi    r11,r11, <low> */
>   /* Save current r2 value in magic place on the stack. */
>   0xf8410000|R2_STACK_OFFSET, /* std     r2,R2_STACK_OFFSET(r1) */
> - 0xe98b0020, /* ld      r12,32(r11) */
> + 0xe98b0040, /* ld      r12,64(r11) */
>  #ifdef PPC64_ELF_ABI_v1
>   /* Set up new r2 from function descriptor */
>   0xe84b0028, /* ld      r2,40(r11) */
> @@ -140,6 +146,24 @@ static u32 ppc64_stub_insns[] = {
>   0x4e800420 /* bctr */
>  };
>
> +static u32 ppc64_klp_stub_insns[] = {
> + 0x3d620000,                     /* addis   r11,r2, <high> */
> + 0x396b0000,                     /* addi    r11,r11, <low> */
> + 0x7c0802a6,                     /* mflr    r0 */
> + 0xf8010010,                     /* std     r0,16(r1) */
> + /* Save current r2 value in magic place on the stack. */
> + 0xf8410000|R2_STACK_OFFSET,     /* std     r2,R2_STACK_OFFSET(r1) */
> + 0xe98b0040, /* ld   r12,64(r11) */
> + 0xf821ffe1,                     /* stdu    r1,-32(r1) */
> + 0x7d8903a6,                     /* mtctr   r12 */
> + 0x4e800421,                     /* bctrl */
> + 0x38210020,                     /* addi    r1,r1,32 */
> + 0xe8010010,                     /* ld      r0,16(r1) */
> + 0xe8410000|R2_STACK_OFFSET,     /* ld      r2,R2_STACK_OFFSET(r1) */
> + 0x7c0803a6,                     /* mtlr    r0 */
> + 0x4e800020                      /* blr */
> +};
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  int module_trampoline_target(struct module *mod, unsigned long addr,
>       unsigned long *target)
> @@ -392,11 +416,16 @@ static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module *me)
>  static inline int create_stub(const Elf64_Shdr *sechdrs,
>        struct ppc64_stub_entry *entry,
>        unsigned long addr,
> -      struct module *me)
> +      struct module *me, int klp_symbol)
>  {
>   long reladdr;
>
> - memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
> + memset(entry, 0, sizeof(*entry));
> + if (klp_symbol)
> + memcpy(entry->jump, ppc64_klp_stub_insns,
> + sizeof(ppc64_klp_stub_insns));
> + else
> + memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
>
>   /* Stub uses address relative to r2. */
>   reladdr = (unsigned long)entry - my_r2(sechdrs, me);
> @@ -419,7 +448,7 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
>     stub to set up the TOC ptr (r2) for the function. */
>  static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
>     unsigned long addr,
> -   struct module *me)
> +   struct module *me, int klp_symbol)
>  {
>   struct ppc64_stub_entry *stubs;
>   unsigned int i, num_stubs;
> @@ -435,7 +464,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
>   return (unsigned long)&stubs[i];
>   }
>
> - if (!create_stub(sechdrs, &stubs[i], addr, me))
> + if (!create_stub(sechdrs, &stubs[i], addr, me, klp_symbol))
>   return 0;
>
>   return (unsigned long)&stubs[i];
> @@ -615,13 +644,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>   /* FIXME: Handle weak symbols here --RR */
>   if (sym->st_shndx == SHN_UNDEF) {
>   /* External: go via stub */
> - value = stub_for_addr(sechdrs, value, me);
> + value = stub_for_addr(sechdrs, value, me, 0);
>   if (!value)
>   return -ENOENT;
>   if (!restore_r2((u32 *)location + 1, me))
>   return -ENOEXEC;
>
>   squash_toc_save_inst(strtab + sym->st_name, value);
> + } else if (sym->st_shndx == SHN_LIVEPATCH) {
> + value = stub_for_addr(sechdrs, value, me, 1);
> + if (!value)
> + return -ENOENT;
>   } else
>   value += local_entry_offset(sym);
>
> @@ -775,7 +808,7 @@ static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module
>  #else
>  static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
>  {
> - return stub_for_addr(sechdrs, (unsigned long)ftrace_caller, me);
> + return stub_for_addr(sechdrs, (unsigned long)ftrace_caller, me, 0);
>  }
>  #endif
>


--
cheers,
Kamalesh.