From 495e6197508dab6cccb6ebd3672c9db358a0540e Mon Sep 17 00:00:00 2001 From: Evgenii Shatokhin Date: Fri, 17 Nov 2017 17:57:06 +0300 Subject: [PATCH 1/2] kpatch-build, x86: do not use the patched functions as callbacks directly A kernel crash happened in __do_softirq() in very rare cases when the binary patch created from mainline commit be82485fbcbb ("netlink: fix an use-after-free issue for nlk groups") was unloaded. Investigation has shown that the kernel tried to execute an RCU callback, deferred_put_nlk_sk(), defined in the patch module after the module had been unloaded. The callback was set by the patched variant of netlink_release() and the address of the patched deferred_put_nlk_sk() was used, rather than the address of the original function. Similar problems occur with workqueue functions as well. As suggested in https://github.com/dynup/kpatch/pull/755#issuecomment-344135224, create-diff-object was modified so that the addresses of the original functions were used in such situations, at least for x86 systems. A similar fix for PowerPC was added as well. Changes in v4: * '#ifdef __x86_64__' was removed. It is not actually needed right now because the constants for relocation types are different on different architectures. Changes in v3: * Minor refactoring and a comment explaining what this all is about. Quite lengthy, but the dynrela-related code is really far from obvious. Changes in v2: * Handle the nested functions the same way as before, because they are unlikely to be used as asynchronous callbacks. Example: cmp() in bch_cache_show() from drivers/md/bcache/sysfs.c in the kernel 4.4. As the nested functions are local to the functions they are defined in, the compiler names them in a similar way to static locals: .. Currently, we filter out all functions with '.' in their names. If there are any asynchronous callbacks in the kernel that have a dot in their names too, they could be handled in the future patches. It is unclear though, if the callbacks with such names can appear in the kernel. Signed-off-by: Evgenii Shatokhin --- kpatch-build/create-diff-object.c | 62 ++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index 6070e68..47cb1d4 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -2328,6 +2328,55 @@ static int kpatch_is_core_module_symbol(char *name) !strcmp(name, "kpatch_shadow_get")); } +/* + * If the patched code refers to a symbol, for example, calls a function + * or stores a pointer to a function somewhere, the address of that symbol + * must be resolved somehow before the patch is applied. The symbol may be + * present in the original code too, so the patch may refer either to that + * version of the symbol (dynrela is used for that) or to its patched + * version directly (with a normal relocation). + * + * Dynrelas may be needed for the symbols not present in this object file + * (rela->sym->sec is NULL), because it is unknown if the patched versions + * of these symbols exist and where they are. + * + * The patched code can usually refer to a symbol from this object file + * directly. If it is a function, this may also improve performance because + * it will not be needed to call the original function first, find the + * patched one and then use Ftrace to pass control to it. + * + * There is an exception though, at least on x86. It is safer to use + * a dynrela if the patched code stores a pointer to a function somewhere + * (relocation of type R_X86_64_32S). The function could be used as + * a callback and some kinds of callbacks are called asynchronously. If + * the patch module sets such callback and is unloaded shortly after, + * the kernel could try to call the function via an invalid pointer and + * would crash. With dynrela, the kernel would call the original function + * in that case. + * + * Nested functions used as callbacks are a special case. They are not + * supposed to be visible outside of the function that defines them. + * Their names may differ in the original and the patched kernels which + * makes it difficult to use dynrelas. Fortunately, nested functions are + * rare and are unlikely to be used as asynchronous callbacks, so the + * patched code can refer to them directly. It seems, one can only + * distinguish such functions by their names containing a dot. Other + * kinds of functions with such names (e.g. optimized copies of functions) + * are unlikely to be used as callbacks. + */ +static int function_ptr_rela(const struct rela *rela) +{ + return (rela->sym->type == STT_FUNC && rela->type == R_X86_64_32S); +} + +static int may_need_dynrela(const struct rela *rela) +{ + if (!rela->sym->sec) + return 1; + + return (function_ptr_rela(rela) && !strchr(rela->sym->name, '.')); +} + static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf, struct lookup_table *table, char *objname, @@ -2386,8 +2435,9 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf, !strcmp(sec->name, ".rela.kpatch.dynrelas")) continue; list_for_each_entry_safe(rela, safe, &sec->relas, list) { - if (rela->sym->sec) + if (!may_need_dynrela(rela)) continue; + /* * Allow references to core module symbols to remain as * normal relas, since the core module may not be @@ -2576,7 +2626,15 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf, rela2->offset = index * sizeof(*krelas) + \ offsetof(struct kpatch_relocation, ksym); - rela->sym->strip = 1; + /* + * Mark the referred to symbol for removal but + * only if it is not from this object file. + * The symbols from this object file may be needed + * later (for example, they may have relocations + * of their own which should be processed). + */ + if (!rela->sym->sec) + rela->sym->strip = 1; list_del(&rela->list); free(rela); From 19b0aba67257296d2c5d335f61aaff6e7049e287 Mon Sep 17 00:00:00 2001 From: Kamalesh Babulal Date: Wed, 21 Mar 2018 09:05:25 +0530 Subject: [PATCH 2/2] PPC64le - do not use the patched functions as callbacks directly It was observed by Evgenii Shatokhin in PR#755, that when the RCU callback was called on the patched function, from unloaded livepatch module triggered a kernel crash. This patch implements the approach on PowerPC outlined in PR#755. With -mcmodel=large, like any other data, function pointers are also loaded relative to the current TOC base and are populated as relocation entries in .toc section. Every function passing a function pointer as the argument need to load the function address through .toc section + offset. Convert such .toc + offset relocation into a dynamic rela, which resolves to original function address, during module load. Also move the comment related to nested function check, into may_need_dynrela(). Suggested-by: Josh Poimboeuf Cc: Evgenii Shatokhin Cc: Joe Lawrence Signed-off-by: Kamalesh Babulal --- kpatch-build/create-diff-object.c | 70 ++++++++++++++++++++++++------- kpatch-build/kpatch-elf.h | 2 + 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index 47cb1d4..1c836ef 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -2328,6 +2328,16 @@ static int kpatch_is_core_module_symbol(char *name) !strcmp(name, "kpatch_shadow_get")); } +static struct rela *toc_rela(const struct rela *rela) +{ + if (rela->type != R_PPC64_TOC16_HA && + rela->type != R_PPC64_TOC16_LO_DS) + return (struct rela *)rela; + + /* Will return NULL for .toc constant entries */ + return find_rela_by_offset(rela->sym->sec->rela, rela->addend); +} + /* * If the patched code refers to a symbol, for example, calls a function * or stores a pointer to a function somewhere, the address of that symbol @@ -2353,28 +2363,38 @@ static int kpatch_is_core_module_symbol(char *name) * the kernel could try to call the function via an invalid pointer and * would crash. With dynrela, the kernel would call the original function * in that case. - * - * Nested functions used as callbacks are a special case. They are not - * supposed to be visible outside of the function that defines them. - * Their names may differ in the original and the patched kernels which - * makes it difficult to use dynrelas. Fortunately, nested functions are - * rare and are unlikely to be used as asynchronous callbacks, so the - * patched code can refer to them directly. It seems, one can only - * distinguish such functions by their names containing a dot. Other - * kinds of functions with such names (e.g. optimized copies of functions) - * are unlikely to be used as callbacks. */ static int function_ptr_rela(const struct rela *rela) { - return (rela->sym->type == STT_FUNC && rela->type == R_X86_64_32S); + const struct rela *rela_toc = toc_rela(rela); + + return (rela_toc && rela_toc->sym->type == STT_FUNC && + /* skip switch table on PowerPC */ + rela_toc->addend == rela_toc->sym->sym.st_value && + (rela->type == R_X86_64_32S || + rela->type == R_PPC64_TOC16_HA || + rela->type == R_PPC64_TOC16_LO_DS)); } static int may_need_dynrela(const struct rela *rela) { if (!rela->sym->sec) return 1; - - return (function_ptr_rela(rela) && !strchr(rela->sym->name, '.')); + /* + * Nested functions used as callbacks are a special case. + * They are not supposed to be visible outside of the + * function that defines them. Their names may differ in + * the original and the patched kernels which makes it + * difficult to use dynrelas. Fortunately, nested functions + * are rare and are unlikely to be used as asynchronous + * callbacks, so the patched code can refer to them directly. + * It seems, one can only distinguish such functions by their + * names containing a dot. Other kinds of functions with + * such names (e.g. optimized copies of functions) are + * unlikely to be used as callbacks. + */ + return (function_ptr_rela(rela) && + !strchr(toc_rela(rela)->sym->name, '.')); } static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf, @@ -2401,8 +2421,28 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf, continue; if (!strcmp(sec->name, ".rela.kpatch.funcs")) continue; - list_for_each_entry(rela, &sec->relas, list) + list_for_each_entry(rela, &sec->relas, list) { nr++; /* upper bound on number of kpatch relas and symbols */ + /* + * Relocation section '.rela.toc' at offset 0xcc6b0 contains 46 entries: + * ... + * 0000000000000138 0000002a00000026 R_PPC64_ADDR64 0000000000000000 .text.deferred_put_nlk_sk + 8 + * + * Relocation section '.rela.text.netlink_release' at offset 0xcadf0 contains 44 entries: + * ... + * 0000000000000398 0000007300000032 R_PPC64_TOC16_HA 0000000000000000 .toc + 138 + * 00000000000003a0 0000007300000040 R_PPC64_TOC16_LO_DS 0000000000000000 .toc + 138 + * + * On PowerPC, may_need_dynrela() should be using rela's reference in .rela.toc for + * the rela like in the example, where the sym name is .toc + offset. In such case, + * the checks are performed on both rela and its reference in .rela.toc. Where the + * rela is checked for rela->type and its corresponding rela in .rela.toc for function + * pointer/switch label. If rela->need_dynrela needs to be set, it's referenced rela + * in (.rela.toc)->need_dynrela is set, as they represent the function sym. + */ + if (may_need_dynrela(rela)) + toc_rela(rela)->need_dynrela = 1; + } } /* create .kpatch.relocations text/rela section pair */ @@ -2435,7 +2475,7 @@ static void kpatch_create_intermediate_sections(struct kpatch_elf *kelf, !strcmp(sec->name, ".rela.kpatch.dynrelas")) continue; list_for_each_entry_safe(rela, safe, &sec->relas, list) { - if (!may_need_dynrela(rela)) + if (!rela->need_dynrela) continue; /* diff --git a/kpatch-build/kpatch-elf.h b/kpatch-build/kpatch-elf.h index e4b5b52..1f059bc 100644 --- a/kpatch-build/kpatch-elf.h +++ b/kpatch-build/kpatch-elf.h @@ -20,6 +20,7 @@ #ifndef _KPATCH_ELF_H_ #define _KPATCH_ELF_H_ +#include #include #include "list.h" #include "log.h" @@ -90,6 +91,7 @@ struct rela { int addend; int offset; char *string; + bool need_dynrela; }; struct string {