Currently, patching objects with duplicate symbol names fail because the
creation of the sysfs function directory collides with the previous
attempt. Appending old_addr to the function name is problematic as it
reveals the address of the function being patch to a normal user. Using
the symbol's occurrence in kallsyms to postfix the function name in the
sysfs directory solves the issue of having consistent unique names and
ensuring that the address is not exposed to a normal user.
In addition, using the symbol position as the user's method to disambiguate
symbols instead of addr allows for disambiguating symbols in modules as
well for both function addresses and for relocs. This also simplifies much
of the code. Special handling for kASLR is no longer needed and can be
removed. The klp_find_verify_func_addr function can be replaced by
klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can
be removed completely.
The following set of patches use symbol positioning instead of old
addresses to disambiguate symbols that have the same name in a given
object. This is necessary in order to be able to patch symbols with the
same name within the same object. This requires modifications to the
klp_func and klp_reloc structures to add an additional element. In addition
the scheme used for the func directory in sysfs is modified to append the
symbols occurrence in kallsyms.
v8:
- explain motivation and necessity in initial patch
- don't support sympos for external relocations
v7:
- make count/pos checking in klp_find_callback more readable
- fix klp_write_object_relocations sympos/external checking
v6:
- move sympos arg before addr in klp_find_object_symbol
- make comments more accurate, remove unnecessary whitespace
- improve cover letter
v5:
- remove val from klp_reloc struct
- klp_write_object_relocations doesn't use sympos with external relocs
- add Petr Mladek's patch to simplify relocated external symbol code
- add optimization in klp_find_callback in unique case
- remove klp_find_verify_func_addr
- amend/remove commit messages/comments to be more precise
Chris J Arges (3):
livepatch: add old_sympos as disambiguator field to klp_func
livepatch: add sympos as disambiguator field to klp_reloc
livepatch: function,sympos scheme in livepatch sysfs directory
Documentation/ABI/testing/sysfs-kernel-livepatch | 6 +-
include/linux/livepatch.h | 24 ++--
kernel/livepatch/core.c | 157 ++++++++---------------
3 files changed, 73 insertions(+), 114 deletions(-)
--
1.9.1
Currently, patching objects with duplicate symbol names fail because the
creation of the sysfs function directory collides with the previous
attempt. Appending old_addr to the function name is problematic as it
reveals the address of the function being patch to a normal user. Using
the symbol's occurrence in kallsyms to postfix the function name in the
sysfs directory solves the issue of having consistent unique names and
ensuring that the address is not exposed to a normal user.
In addition, using the symbol position as the user's method to disambiguate
symbols instead of addr allows for disambiguating symbols in modules as
well for both function addresses and for relocs. This also simplifies much
of the code. Special handling for kASLR is no longer needed and can be
removed. The klp_find_verify_func_addr function can be replaced by
klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can
be removed completely.
In cases of duplicate symbols, old_sympos will be used to disambiguate
instead of old_addr. By default old_sympos will be 0, and patching will
only succeed if the symbol is unique. Specifying a positive value will
ensure that occurrence of the symbol in kallsyms for the patched object
will be used for patching if it is valid.
In addition, make old_addr an internal structure field not to be specified
by the user. Finally, remove klp_find_verify_func_addr as it can be
replaced by klp_find_object_symbol directly.
Support for symbol position disambiguation for relocations is added in the
next patch in this series.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 19 +++++++------
kernel/livepatch/core.c | 72 ++++++++++++++++++++---------------------------
2 files changed, 41 insertions(+), 50 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..b60e8ab 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,8 +37,9 @@ enum klp_state {
* struct klp_func - function structure for live patching
* @old_name: name of the function to be patched
* @new_func: pointer to the patched function code
- * @old_addr: a hint conveying at what address the old function
- * can be found (optional, vmlinux patches only)
+ * @old_sympos: a hint indicating which symbol position the old function
+ * can be found (optional)
+ * @old_addr: the address of the function being patched
* @kobj: kobject for sysfs resources
* @state: tracks function-level patch application state
* @stack_node: list node for klp_ops func_stack list
@@ -48,16 +49,16 @@ struct klp_func {
const char *old_name;
void *new_func;
/*
- * The old_addr field is optional and can be used to resolve
- * duplicate symbol names in the vmlinux object. If this
- * information is not present, the symbol is located by name
- * with kallsyms. If the name is not unique and old_addr is
- * not provided, the patch application fails as there is no
- * way to resolve the ambiguity.
+ * The old_sympos field is optional and can be used to resolve
+ * duplicate symbol names in livepatch objects. If this field is zero,
+ * it is expected the symbol is unique, otherwise patching fails. If
+ * this value is greater than zero then that occurrence of the symbol
+ * in kallsyms for the given object is used.
*/
- unsigned long old_addr;
+ unsigned long old_sympos;
/* internal */
+ unsigned long old_addr;
struct kobject kobj;
enum klp_state state;
struct list_head stack_node;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..5a53383 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -135,13 +135,8 @@ struct klp_find_arg {
const char *objname;
const char *name;
unsigned long addr;
- /*
- * If count == 0, the symbol was not found. If count == 1, a unique
- * match was found and addr is set. If count > 1, there is
- * unresolvable ambiguity among "count" number of symbols with the same
- * name in the same object.
- */
unsigned long count;
+ unsigned long pos;
};
static int klp_find_callback(void *data, const char *name,
@@ -158,37 +153,48 @@ static int klp_find_callback(void *data, const char *name,
if (args->objname && strcmp(args->objname, mod->name))
return 0;
- /*
- * args->addr might be overwritten if another match is found
- * but klp_find_object_symbol() handles this and only returns the
- * addr if count == 1.
- */
args->addr = addr;
args->count++;
+ /*
+ * Finish the search when the symbol is found for the desired position
+ * or the position is not defined for a non-unique symbol.
+ */
+ if ((args->pos && (args->count == args->pos)) ||
+ (!args->pos && (args->count > 1)))
+ return 1;
+
return 0;
}
static int klp_find_object_symbol(const char *objname, const char *name,
- unsigned long *addr)
+ unsigned long sympos, unsigned long *addr)
{
struct klp_find_arg args = {
.objname = objname,
.name = name,
.addr = 0,
- .count = 0
+ .count = 0,
+ .pos = sympos,
};
mutex_lock(&module_mutex);
kallsyms_on_each_symbol(klp_find_callback, &args);
mutex_unlock(&module_mutex);
- if (args.count == 0)
+ /*
+ * Ensure an address was found. If sympos is 0, ensure symbol is unique;
+ * otherwise ensure the symbol position count matches sympos.
+ */
+ if (args.addr == 0)
pr_err("symbol '%s' not found in symbol table\n", name);
- else if (args.count > 1)
+ else if (args.count > 1 && sympos == 0) {
pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
args.count, name, objname);
- else {
+ } else if (sympos != args.count && sympos > 0) {
+ pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
+ sympos, name, objname ? objname : "vmlinux");
+ } else {
*addr = args.addr;
return 0;
}
@@ -236,27 +242,6 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
return 0;
}
-static int klp_find_verify_func_addr(struct klp_object *obj,
- struct klp_func *func)
-{
- int ret;
-
-#if defined(CONFIG_RANDOMIZE_BASE)
- /* If KASLR has been enabled, adjust old_addr accordingly */
- if (kaslr_enabled() && func->old_addr)
- func->old_addr += kaslr_offset();
-#endif
-
- if (!func->old_addr || klp_is_module(obj))
- ret = klp_find_object_symbol(obj->name, func->old_name,
- &func->old_addr);
- else
- ret = klp_verify_vmlinux_symbol(func->old_name,
- func->old_addr);
-
- return ret;
-}
-
/*
* external symbols are located outside the parent object (where the parent
* object is either vmlinux or the kmod being patched).
@@ -276,8 +261,11 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
}
preempt_enable();
- /* otherwise check if it's in another .o within the patch module */
- return klp_find_object_symbol(pmod->name, name, addr);
+ /*
+ * Check if it's in another .o within the patch module. This also
+ * checks that the external symbol is unique.
+ */
+ return klp_find_object_symbol(pmod->name, name, 0, addr);
}
static int klp_write_object_relocations(struct module *pmod,
@@ -307,7 +295,7 @@ static int klp_write_object_relocations(struct module *pmod,
else
ret = klp_find_object_symbol(obj->mod->name,
reloc->name,
- &reloc->val);
+ 0, &reloc->val);
if (ret)
return ret;
}
@@ -750,7 +738,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
}
klp_for_each_func(obj, func) {
- ret = klp_find_verify_func_addr(obj, func);
+ ret = klp_find_object_symbol(obj->name, func->old_name,
+ func->old_sympos,
+ &func->old_addr);
if (ret)
return ret;
}
--
1.9.1
In cases of duplicate symbols, sympos will be used to disambiguate instead
of val. By default sympos will be 0, and patching will only succeed if
the symbol is unique. Specifying a positive value will ensure that
occurrence of the symbol in kallsyms for the patched object will be used
for patching if it is valid. For external relocations sympos is not
supported.
Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
as they are no longer used.
>From the klp_reloc structure remove val, as it can be refactored as a
local variable in klp_write_object_relocations.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 5 ++-
kernel/livepatch/core.c | 77 +++++++++++------------------------------------
2 files changed, 20 insertions(+), 62 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index b60e8ab..a882865 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -67,8 +67,7 @@ struct klp_func {
/**
* struct klp_reloc - relocation structure for live patching
* @loc: address where the relocation will be written
- * @val: address of the referenced symbol (optional,
- * vmlinux patches only)
+ * @sympos: position in kallsyms to disambiguate symbols (optional)
* @type: ELF relocation type
* @name: name of the referenced symbol (for lookup/verification)
* @addend: offset from the referenced symbol
@@ -76,7 +75,7 @@ struct klp_func {
*/
struct klp_reloc {
unsigned long loc;
- unsigned long val;
+ unsigned long sympos;
unsigned long type;
const char *name;
int addend;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5a53383..c9f924e 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -203,45 +203,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
return -EINVAL;
}
-struct klp_verify_args {
- const char *name;
- const unsigned long addr;
-};
-
-static int klp_verify_callback(void *data, const char *name,
- struct module *mod, unsigned long addr)
-{
- struct klp_verify_args *args = data;
-
- if (!mod &&
- !strcmp(args->name, name) &&
- args->addr == addr)
- return 1;
-
- return 0;
-}
-
-static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
-{
- struct klp_verify_args args = {
- .name = name,
- .addr = addr,
- };
- int ret;
-
- mutex_lock(&module_mutex);
- ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
- mutex_unlock(&module_mutex);
-
- if (!ret) {
- pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
- name, addr);
- return -EINVAL;
- }
-
- return 0;
-}
-
/*
* external symbols are located outside the parent object (where the parent
* object is either vmlinux or the kmod being patched).
@@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
struct klp_object *obj)
{
int ret;
+ unsigned long val;
struct klp_reloc *reloc;
if (WARN_ON(!klp_is_object_loaded(obj)))
@@ -281,29 +243,26 @@ static int klp_write_object_relocations(struct module *pmod,
return -EINVAL;
for (reloc = obj->relocs; reloc->name; reloc++) {
- if (!klp_is_module(obj)) {
- ret = klp_verify_vmlinux_symbol(reloc->name,
- reloc->val);
- if (ret)
- return ret;
- } else {
- /* module, reloc->val needs to be discovered */
- if (reloc->external)
- ret = klp_find_external_symbol(pmod,
- reloc->name,
- &reloc->val);
- else
- ret = klp_find_object_symbol(obj->mod->name,
- reloc->name,
- 0, &reloc->val);
- if (ret)
- return ret;
- }
+ /* discover the address of the referenced symbol */
+ if (reloc->external) {
+ if (reloc->sympos > 0) {
+ pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
+ reloc->name);
+ return -EINVAL;
+ }
+ ret = klp_find_external_symbol(pmod, reloc->name, &val);
+ } else
+ ret = klp_find_object_symbol(obj->mod->name,
+ reloc->name,
+ reloc->sympos,
+ &val);
+ if (ret)
+ return ret;
ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
- reloc->val + reloc->addend);
+ val + reloc->addend);
if (ret) {
pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
- reloc->name, reloc->val, ret);
+ reloc->name, val, ret);
return ret;
}
}
--
1.9.1
The following directory structure will allow for cases when the same
function name exists in a single object.
/sys/kernel/livepatch/<patch>/<object>/<function,sympos>
The sympos number corresponds to the nth occurrence of the symbol name in
kallsyms for the patched object.
An example of patching multiple symbols can be found here:
https://github.com/dynup/kpatch/issues/493
Signed-off-by: Chris J Arges <[email protected]>
---
Documentation/ABI/testing/sysfs-kernel-livepatch | 6 +++++-
kernel/livepatch/core.c | 10 ++++++++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 5bf42a8..da87f43 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,7 +33,7 @@ Description:
The object directory contains subdirectories for each function
that is patched within the object.
-What: /sys/kernel/livepatch/<patch>/<object>/<function>
+What: /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
Date: Nov 2014
KernelVersion: 3.19.0
Contact: [email protected]
@@ -41,4 +41,8 @@ Description:
The function directory contains attributes regarding the
properties and state of the patched function.
+ The directory name contains the patched function name and a
+ sympos number corresponding to the nth occurrence of the symbol
+ name in kallsyms for the patched object.
+
There are currently no such attributes.
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index c9f924e..0e86c45 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -534,7 +534,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
* /sys/kernel/livepatch/<patch>
* /sys/kernel/livepatch/<patch>/enabled
* /sys/kernel/livepatch/<patch>/<object>
- * /sys/kernel/livepatch/<patch>/<object>/<func>
+ * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
*/
static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -679,8 +679,14 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
INIT_LIST_HEAD(&func->stack_node);
func->state = KLP_DISABLED;
+ /* The format for the sysfs directory is <function,sympos> where sympos
+ * is the nth occurrence of this symbol in kallsyms for the patched
+ * object. If the user selects 0 for old_sympos, then 1 will be used
+ * since a unique symbol will be the first occurrence.
+ */
return kobject_init_and_add(&func->kobj, &klp_ktype_func,
- &obj->kobj, "%s", func->old_name);
+ &obj->kobj, "%s,%lu", func->old_name,
+ func->old_sympos ? func->old_sympos : 1);
}
/* parts of the initialization that is done only when the object is loaded */
--
1.9.1
On Fri, 20 Nov 2015, Chris J Arges wrote:
> Currently, patching objects with duplicate symbol names fail because the
> creation of the sysfs function directory collides with the previous
> attempt. Appending old_addr to the function name is problematic as it
> reveals the address of the function being patch to a normal user. Using
> the symbol's occurrence in kallsyms to postfix the function name in the
> sysfs directory solves the issue of having consistent unique names and
> ensuring that the address is not exposed to a normal user.
>
> In addition, using the symbol position as the user's method to disambiguate
> symbols instead of addr allows for disambiguating symbols in modules as
> well for both function addresses and for relocs. This also simplifies much
> of the code. Special handling for kASLR is no longer needed and can be
> removed. The klp_find_verify_func_addr function can be replaced by
> klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can
> be removed completely.
>
> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. By default old_sympos will be 0, and patching will
> only succeed if the symbol is unique. Specifying a positive value will
> ensure that occurrence of the symbol in kallsyms for the patched object
> will be used for patching if it is valid.
>
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
>
> Support for symbol position disambiguation for relocations is added in the
> next patch in this series.
>
> Signed-off-by: Chris J Arges <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
Miroslav
On Fri, 20 Nov 2015, Chris J Arges wrote:
> The following directory structure will allow for cases when the same
> function name exists in a single object.
> /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
>
> The sympos number corresponds to the nth occurrence of the symbol name in
> kallsyms for the patched object.
>
> An example of patching multiple symbols can be found here:
> https://github.com/dynup/kpatch/issues/493
>
> Signed-off-by: Chris J Arges <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
Miroslav
On Fri, 20 Nov 2015, Chris J Arges wrote:
[...]
> @@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
> struct klp_object *obj)
> {
> int ret;
> + unsigned long val;
> struct klp_reloc *reloc;
>
> if (WARN_ON(!klp_is_object_loaded(obj)))
> @@ -281,29 +243,26 @@ static int klp_write_object_relocations(struct module *pmod,
> return -EINVAL;
>
> for (reloc = obj->relocs; reloc->name; reloc++) {
> - if (!klp_is_module(obj)) {
> - ret = klp_verify_vmlinux_symbol(reloc->name,
> - reloc->val);
> - if (ret)
> - return ret;
> - } else {
> - /* module, reloc->val needs to be discovered */
> - if (reloc->external)
> - ret = klp_find_external_symbol(pmod,
> - reloc->name,
> - &reloc->val);
> - else
> - ret = klp_find_object_symbol(obj->mod->name,
> - reloc->name,
> - 0, &reloc->val);
> - if (ret)
> - return ret;
> - }
> + /* discover the address of the referenced symbol */
> + if (reloc->external) {
> + if (reloc->sympos > 0) {
> + pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> + reloc->name);
> + return -EINVAL;
> + }
> + ret = klp_find_external_symbol(pmod, reloc->name, &val);
> + } else
> + ret = klp_find_object_symbol(obj->mod->name,
> + reloc->name,
> + reloc->sympos,
> + &val);
I think this will cause an unwanted effect if a relocation is for vmlinux
symbol. obj->mod is NULL in that case. So obj->name should be there. Can
you test it with kpatch, Chris, please? If that is possible.
Thanks,
Miroslav
On Mon, Nov 23, 2015 at 10:52:23AM +0100, Miroslav Benes wrote:
> On Fri, 20 Nov 2015, Chris J Arges wrote:
>
> [...]
>
> > @@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
> > struct klp_object *obj)
> > {
> > int ret;
> > + unsigned long val;
> > struct klp_reloc *reloc;
> >
> > if (WARN_ON(!klp_is_object_loaded(obj)))
> > @@ -281,29 +243,26 @@ static int klp_write_object_relocations(struct module *pmod,
> > return -EINVAL;
> >
> > for (reloc = obj->relocs; reloc->name; reloc++) {
> > - if (!klp_is_module(obj)) {
> > - ret = klp_verify_vmlinux_symbol(reloc->name,
> > - reloc->val);
> > - if (ret)
> > - return ret;
> > - } else {
> > - /* module, reloc->val needs to be discovered */
> > - if (reloc->external)
> > - ret = klp_find_external_symbol(pmod,
> > - reloc->name,
> > - &reloc->val);
> > - else
> > - ret = klp_find_object_symbol(obj->mod->name,
> > - reloc->name,
> > - 0, &reloc->val);
> > - if (ret)
> > - return ret;
> > - }
> > + /* discover the address of the referenced symbol */
> > + if (reloc->external) {
> > + if (reloc->sympos > 0) {
> > + pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> > + reloc->name);
> > + return -EINVAL;
> > + }
> > + ret = klp_find_external_symbol(pmod, reloc->name, &val);
> > + } else
> > + ret = klp_find_object_symbol(obj->mod->name,
> > + reloc->name,
> > + reloc->sympos,
> > + &val);
>
> I think this will cause an unwanted effect if a relocation is for vmlinux
> symbol. obj->mod is NULL in that case. So obj->name should be there. Can
> you test it with kpatch, Chris, please? If that is possible.
>
> Thanks,
> Miroslav
>
Yes I need to fix this, an option would be to use:
ret = klp_find_object_symbol(obj->name ? obj->name : obj->mod->name,
reloc->name, 0, &reloc->val);
Testing with kpatch will require some modifications as the structures are
different.
--chris
On Mon, Nov 30, 2015 at 02:46:18PM -0600, Chris J Arges wrote:
> On Mon, Nov 23, 2015 at 10:52:23AM +0100, Miroslav Benes wrote:
> > On Fri, 20 Nov 2015, Chris J Arges wrote:
> >
> > [...]
> >
> > > @@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
> > > struct klp_object *obj)
> > > {
> > > int ret;
> > > + unsigned long val;
> > > struct klp_reloc *reloc;
> > >
> > > if (WARN_ON(!klp_is_object_loaded(obj)))
> > > @@ -281,29 +243,26 @@ static int klp_write_object_relocations(struct module *pmod,
> > > return -EINVAL;
> > >
> > > for (reloc = obj->relocs; reloc->name; reloc++) {
> > > - if (!klp_is_module(obj)) {
> > > - ret = klp_verify_vmlinux_symbol(reloc->name,
> > > - reloc->val);
> > > - if (ret)
> > > - return ret;
> > > - } else {
> > > - /* module, reloc->val needs to be discovered */
> > > - if (reloc->external)
> > > - ret = klp_find_external_symbol(pmod,
> > > - reloc->name,
> > > - &reloc->val);
> > > - else
> > > - ret = klp_find_object_symbol(obj->mod->name,
> > > - reloc->name,
> > > - 0, &reloc->val);
> > > - if (ret)
> > > - return ret;
> > > - }
> > > + /* discover the address of the referenced symbol */
> > > + if (reloc->external) {
> > > + if (reloc->sympos > 0) {
> > > + pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> > > + reloc->name);
> > > + return -EINVAL;
> > > + }
> > > + ret = klp_find_external_symbol(pmod, reloc->name, &val);
> > > + } else
> > > + ret = klp_find_object_symbol(obj->mod->name,
> > > + reloc->name,
> > > + reloc->sympos,
> > > + &val);
> >
> > I think this will cause an unwanted effect if a relocation is for vmlinux
> > symbol. obj->mod is NULL in that case. So obj->name should be there. Can
> > you test it with kpatch, Chris, please? If that is possible.
> >
> > Thanks,
> > Miroslav
> >
>
> Yes I need to fix this, an option would be to use:
> ret = klp_find_object_symbol(obj->name ? obj->name : obj->mod->name,
> reloc->name, 0, &reloc->val);
Instead of
'obj->name ? obj->name : obj->mod->name'
I think it can just be 'obj->name'.
If it's patching a module, obj->name will be the module name, otherwise
it will be NULL (for patching vmlinux). klp_find_object_symbol() is
equipped to handle it either way.
--
Josh
Currently, patching objects with duplicate symbol names fail because the
creation of the sysfs function directory collides with the previous
attempt. Appending old_addr to the function name is problematic as it
reveals the address of the function being patch to a normal user. Using
the symbol's occurrence in kallsyms to postfix the function name in the
sysfs directory solves the issue of having consistent unique names and
ensuring that the address is not exposed to a normal user.
In addition, using the symbol position as the user's method to disambiguate
symbols instead of addr allows for disambiguating symbols in modules as
well for both function addresses and for relocs. This also simplifies much
of the code. Special handling for kASLR is no longer needed and can be
removed. The klp_find_verify_func_addr function can be replaced by
klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can
be removed completely.
The following set of patches use symbol positioning instead of old
addresses to disambiguate symbols that have the same name in a given
object. This is necessary in order to be able to patch symbols with the
same name within the same object. This requires modifications to the
klp_func and klp_reloc structures to add an additional element. In addition
the scheme used for the func directory in sysfs is modified to append the
symbols occurrence in kallsyms.
In addition, I've tested this patchset against a modified kpatch. I
modified the kpatch livepatch scaffolding to set lreloc->sympos = 0, and
lfunc->old_sympos to the nth occurrence of the symbol for that object in
kallsyms. I was able to patch the same test patch as described here:
https://github.com/dynup/kpatch/issues/493
I've also tested this with sample livepatch code to test if the various
old_sympos values work for unique and duplicate functions.
v9:
- use mod->name instead of mod->obj->name for klp_find_object_symbol in
klp_write_object_relocations
- rebase on current master
- tested with kpatch
v8:
- explain motivation and necessity in initial patch
- don't support sympos for external relocations
v7:
- make count/pos checking in klp_find_callback more readable
- fix klp_write_object_relocations sympos/external checking
v6:
- move sympos arg before addr in klp_find_object_symbol
- make comments more accurate, remove unnecessary whitespace
- improve cover letter
v5:
- remove val from klp_reloc struct
- klp_write_object_relocations doesn't use sympos with external relocs
- add Petr Mladek's patch to simplify relocated external symbol code
- add optimization in klp_find_callback in unique case
- remove klp_find_verify_func_addr
- amend/remove commit messages/comments to be more precise
Chris J Arges (3):
livepatch: add old_sympos as disambiguator field to klp_func
livepatch: add sympos as disambiguator field to klp_reloc
livepatch: function,sympos scheme in livepatch sysfs directory
Documentation/ABI/testing/sysfs-kernel-livepatch | 6 +-
include/linux/livepatch.h | 24 ++--
kernel/livepatch/core.c | 164 ++++++++---------------
3 files changed, 74 insertions(+), 120 deletions(-)
--
1.9.1
Currently, patching objects with duplicate symbol names fail because the
creation of the sysfs function directory collides with the previous
attempt. Appending old_addr to the function name is problematic as it
reveals the address of the function being patch to a normal user. Using
the symbol's occurrence in kallsyms to postfix the function name in the
sysfs directory solves the issue of having consistent unique names and
ensuring that the address is not exposed to a normal user.
In addition, using the symbol position as the user's method to disambiguate
symbols instead of addr allows for disambiguating symbols in modules as
well for both function addresses and for relocs. This also simplifies much
of the code. Special handling for kASLR is no longer needed and can be
removed. The klp_find_verify_func_addr function can be replaced by
klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can
be removed completely.
In cases of duplicate symbols, old_sympos will be used to disambiguate
instead of old_addr. By default old_sympos will be 0, and patching will
only succeed if the symbol is unique. Specifying a positive value will
ensure that occurrence of the symbol in kallsyms for the patched object
will be used for patching if it is valid.
In addition, make old_addr an internal structure field not to be specified
by the user. Finally, remove klp_find_verify_func_addr as it can be
replaced by klp_find_object_symbol directly.
Support for symbol position disambiguation for relocations is added in the
next patch in this series.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 19 +++++++------
kernel/livepatch/core.c | 72 ++++++++++++++++++++---------------------------
2 files changed, 41 insertions(+), 50 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..b60e8ab 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,8 +37,9 @@ enum klp_state {
* struct klp_func - function structure for live patching
* @old_name: name of the function to be patched
* @new_func: pointer to the patched function code
- * @old_addr: a hint conveying at what address the old function
- * can be found (optional, vmlinux patches only)
+ * @old_sympos: a hint indicating which symbol position the old function
+ * can be found (optional)
+ * @old_addr: the address of the function being patched
* @kobj: kobject for sysfs resources
* @state: tracks function-level patch application state
* @stack_node: list node for klp_ops func_stack list
@@ -48,16 +49,16 @@ struct klp_func {
const char *old_name;
void *new_func;
/*
- * The old_addr field is optional and can be used to resolve
- * duplicate symbol names in the vmlinux object. If this
- * information is not present, the symbol is located by name
- * with kallsyms. If the name is not unique and old_addr is
- * not provided, the patch application fails as there is no
- * way to resolve the ambiguity.
+ * The old_sympos field is optional and can be used to resolve
+ * duplicate symbol names in livepatch objects. If this field is zero,
+ * it is expected the symbol is unique, otherwise patching fails. If
+ * this value is greater than zero then that occurrence of the symbol
+ * in kallsyms for the given object is used.
*/
- unsigned long old_addr;
+ unsigned long old_sympos;
/* internal */
+ unsigned long old_addr;
struct kobject kobj;
enum klp_state state;
struct list_head stack_node;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index db545cb..e416f96 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -135,13 +135,8 @@ struct klp_find_arg {
const char *objname;
const char *name;
unsigned long addr;
- /*
- * If count == 0, the symbol was not found. If count == 1, a unique
- * match was found and addr is set. If count > 1, there is
- * unresolvable ambiguity among "count" number of symbols with the same
- * name in the same object.
- */
unsigned long count;
+ unsigned long pos;
};
static int klp_find_callback(void *data, const char *name,
@@ -158,37 +153,48 @@ static int klp_find_callback(void *data, const char *name,
if (args->objname && strcmp(args->objname, mod->name))
return 0;
- /*
- * args->addr might be overwritten if another match is found
- * but klp_find_object_symbol() handles this and only returns the
- * addr if count == 1.
- */
args->addr = addr;
args->count++;
+ /*
+ * Finish the search when the symbol is found for the desired position
+ * or the position is not defined for a non-unique symbol.
+ */
+ if ((args->pos && (args->count == args->pos)) ||
+ (!args->pos && (args->count > 1)))
+ return 1;
+
return 0;
}
static int klp_find_object_symbol(const char *objname, const char *name,
- unsigned long *addr)
+ unsigned long sympos, unsigned long *addr)
{
struct klp_find_arg args = {
.objname = objname,
.name = name,
.addr = 0,
- .count = 0
+ .count = 0,
+ .pos = sympos,
};
mutex_lock(&module_mutex);
kallsyms_on_each_symbol(klp_find_callback, &args);
mutex_unlock(&module_mutex);
- if (args.count == 0)
+ /*
+ * Ensure an address was found. If sympos is 0, ensure symbol is unique;
+ * otherwise ensure the symbol position count matches sympos.
+ */
+ if (args.addr == 0)
pr_err("symbol '%s' not found in symbol table\n", name);
- else if (args.count > 1)
+ else if (args.count > 1 && sympos == 0) {
pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
args.count, name, objname);
- else {
+ } else if (sympos != args.count && sympos > 0) {
+ pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
+ sympos, name, objname ? objname : "vmlinux");
+ } else {
*addr = args.addr;
return 0;
}
@@ -236,27 +242,6 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
return 0;
}
-static int klp_find_verify_func_addr(struct klp_object *obj,
- struct klp_func *func)
-{
- int ret;
-
-#if defined(CONFIG_RANDOMIZE_BASE)
- /* If KASLR has been enabled, adjust old_addr accordingly */
- if (kaslr_enabled() && func->old_addr)
- func->old_addr += kaslr_offset();
-#endif
-
- if (!func->old_addr || klp_is_module(obj))
- ret = klp_find_object_symbol(obj->name, func->old_name,
- &func->old_addr);
- else
- ret = klp_verify_vmlinux_symbol(func->old_name,
- func->old_addr);
-
- return ret;
-}
-
/*
* external symbols are located outside the parent object (where the parent
* object is either vmlinux or the kmod being patched).
@@ -276,8 +261,11 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
}
preempt_enable();
- /* otherwise check if it's in another .o within the patch module */
- return klp_find_object_symbol(pmod->name, name, addr);
+ /*
+ * Check if it's in another .o within the patch module. This also
+ * checks that the external symbol is unique.
+ */
+ return klp_find_object_symbol(pmod->name, name, 0, addr);
}
static int klp_write_object_relocations(struct module *pmod,
@@ -313,7 +301,7 @@ static int klp_write_object_relocations(struct module *pmod,
else
ret = klp_find_object_symbol(obj->mod->name,
reloc->name,
- &reloc->val);
+ 0, &reloc->val);
if (ret)
return ret;
}
@@ -756,7 +744,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
}
klp_for_each_func(obj, func) {
- ret = klp_find_verify_func_addr(obj, func);
+ ret = klp_find_object_symbol(obj->name, func->old_name,
+ func->old_sympos,
+ &func->old_addr);
if (ret)
return ret;
}
--
1.9.1
In cases of duplicate symbols, sympos will be used to disambiguate instead
of val. By default sympos will be 0, and patching will only succeed if
the symbol is unique. Specifying a positive value will ensure that
occurrence of the symbol in kallsyms for the patched object will be used
for patching if it is valid. For external relocations sympos is not
supported.
Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
as they are no longer used.
>From the klp_reloc structure remove val, as it can be refactored as a
local variable in klp_write_object_relocations.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 5 ++-
kernel/livepatch/core.c | 84 +++++++++++------------------------------------
2 files changed, 21 insertions(+), 68 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index b60e8ab..a882865 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -67,8 +67,7 @@ struct klp_func {
/**
* struct klp_reloc - relocation structure for live patching
* @loc: address where the relocation will be written
- * @val: address of the referenced symbol (optional,
- * vmlinux patches only)
+ * @sympos: position in kallsyms to disambiguate symbols (optional)
* @type: ELF relocation type
* @name: name of the referenced symbol (for lookup/verification)
* @addend: offset from the referenced symbol
@@ -76,7 +75,7 @@ struct klp_func {
*/
struct klp_reloc {
unsigned long loc;
- unsigned long val;
+ unsigned long sympos;
unsigned long type;
const char *name;
int addend;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e416f96..e842534 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -203,45 +203,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
return -EINVAL;
}
-struct klp_verify_args {
- const char *name;
- const unsigned long addr;
-};
-
-static int klp_verify_callback(void *data, const char *name,
- struct module *mod, unsigned long addr)
-{
- struct klp_verify_args *args = data;
-
- if (!mod &&
- !strcmp(args->name, name) &&
- args->addr == addr)
- return 1;
-
- return 0;
-}
-
-static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
-{
- struct klp_verify_args args = {
- .name = name,
- .addr = addr,
- };
- int ret;
-
- mutex_lock(&module_mutex);
- ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
- mutex_unlock(&module_mutex);
-
- if (!ret) {
- pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
- name, addr);
- return -EINVAL;
- }
-
- return 0;
-}
-
/*
* external symbols are located outside the parent object (where the parent
* object is either vmlinux or the kmod being patched).
@@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
struct klp_object *obj)
{
int ret;
+ unsigned long val;
struct klp_reloc *reloc;
if (WARN_ON(!klp_is_object_loaded(obj)))
@@ -281,35 +243,27 @@ static int klp_write_object_relocations(struct module *pmod,
return -EINVAL;
for (reloc = obj->relocs; reloc->name; reloc++) {
- if (!klp_is_module(obj)) {
-
-#if defined(CONFIG_RANDOMIZE_BASE)
- /* If KASLR has been enabled, adjust old value accordingly */
- if (kaslr_enabled())
- reloc->val += kaslr_offset();
-#endif
- ret = klp_verify_vmlinux_symbol(reloc->name,
- reloc->val);
- if (ret)
- return ret;
- } else {
- /* module, reloc->val needs to be discovered */
- if (reloc->external)
- ret = klp_find_external_symbol(pmod,
- reloc->name,
- &reloc->val);
- else
- ret = klp_find_object_symbol(obj->mod->name,
- reloc->name,
- 0, &reloc->val);
- if (ret)
- return ret;
- }
+ /* discover the address of the referenced symbol */
+ if (reloc->external) {
+ if (reloc->sympos > 0) {
+ pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
+ reloc->name);
+ return -EINVAL;
+ }
+ ret = klp_find_external_symbol(pmod, reloc->name, &val);
+ } else
+ ret = klp_find_object_symbol(obj->name,
+ reloc->name,
+ reloc->sympos,
+ &val);
+ if (ret)
+ return ret;
+
ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
- reloc->val + reloc->addend);
+ val + reloc->addend);
if (ret) {
pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
- reloc->name, reloc->val, ret);
+ reloc->name, val, ret);
return ret;
}
}
--
1.9.1
The following directory structure will allow for cases when the same
function name exists in a single object.
/sys/kernel/livepatch/<patch>/<object>/<function,sympos>
The sympos number corresponds to the nth occurrence of the symbol name in
kallsyms for the patched object.
An example of patching multiple symbols can be found here:
https://github.com/dynup/kpatch/issues/493
Signed-off-by: Chris J Arges <[email protected]>
---
Documentation/ABI/testing/sysfs-kernel-livepatch | 6 +++++-
kernel/livepatch/core.c | 10 ++++++++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 5bf42a8..da87f43 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,7 +33,7 @@ Description:
The object directory contains subdirectories for each function
that is patched within the object.
-What: /sys/kernel/livepatch/<patch>/<object>/<function>
+What: /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
Date: Nov 2014
KernelVersion: 3.19.0
Contact: [email protected]
@@ -41,4 +41,8 @@ Description:
The function directory contains attributes regarding the
properties and state of the patched function.
+ The directory name contains the patched function name and a
+ sympos number corresponding to the nth occurrence of the symbol
+ name in kallsyms for the patched object.
+
There are currently no such attributes.
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e842534..94893e8 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -535,7 +535,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
* /sys/kernel/livepatch/<patch>
* /sys/kernel/livepatch/<patch>/enabled
* /sys/kernel/livepatch/<patch>/<object>
- * /sys/kernel/livepatch/<patch>/<object>/<func>
+ * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
*/
static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -680,8 +680,14 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
INIT_LIST_HEAD(&func->stack_node);
func->state = KLP_DISABLED;
+ /* The format for the sysfs directory is <function,sympos> where sympos
+ * is the nth occurrence of this symbol in kallsyms for the patched
+ * object. If the user selects 0 for old_sympos, then 1 will be used
+ * since a unique symbol will be the first occurrence.
+ */
return kobject_init_and_add(&func->kobj, &klp_ktype_func,
- &obj->kobj, "%s", func->old_name);
+ &obj->kobj, "%s,%lu", func->old_name,
+ func->old_sympos ? func->old_sympos : 1);
}
/* parts of the initialization that is done only when the object is loaded */
--
1.9.1
On Tue 2015-12-01 20:40:53, Chris J Arges wrote:
> v9:
> - use mod->name instead of mod->obj->name for klp_find_object_symbol in
> klp_write_object_relocations
> - rebase on current master
> - tested with kpatch
The last version looks fine to me. Feel free to add the following
into all three patches:
Reviewed-by: Petr Mladek <[email protected]>
Thanks,
Petr
On Tue, Dec 01, 2015 at 08:40:53PM -0600, Chris J Arges wrote:
> Currently, patching objects with duplicate symbol names fail because the
> creation of the sysfs function directory collides with the previous
> attempt. Appending old_addr to the function name is problematic as it
> reveals the address of the function being patch to a normal user. Using
> the symbol's occurrence in kallsyms to postfix the function name in the
> sysfs directory solves the issue of having consistent unique names and
> ensuring that the address is not exposed to a normal user.
>
> In addition, using the symbol position as the user's method to disambiguate
> symbols instead of addr allows for disambiguating symbols in modules as
> well for both function addresses and for relocs. This also simplifies much
> of the code. Special handling for kASLR is no longer needed and can be
> removed. The klp_find_verify_func_addr function can be replaced by
> klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can
> be removed completely.
>
> The following set of patches use symbol positioning instead of old
> addresses to disambiguate symbols that have the same name in a given
> object. This is necessary in order to be able to patch symbols with the
> same name within the same object. This requires modifications to the
> klp_func and klp_reloc structures to add an additional element. In addition
> the scheme used for the func directory in sysfs is modified to append the
> symbols occurrence in kallsyms.
>
> In addition, I've tested this patchset against a modified kpatch. I
> modified the kpatch livepatch scaffolding to set lreloc->sympos = 0, and
> lfunc->old_sympos to the nth occurrence of the symbol for that object in
> kallsyms. I was able to patch the same test patch as described here:
> https://github.com/dynup/kpatch/issues/493
> I've also tested this with sample livepatch code to test if the various
> old_sympos values work for unique and duplicate functions.
>
> v9:
> - use mod->name instead of mod->obj->name for klp_find_object_symbol in
> klp_write_object_relocations
> - rebase on current master
> - tested with kpatch
For the series:
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
Chris,
thanks a lot for promptly reacting to all the feedback.
I've now applied this to livepatching.git#for-4.5/core.
--
Jiri Kosina
SUSE Labs
On Tue, 1 Dec 2015, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default sympos will be 0, and patching will only succeed if
> the symbol is unique. Specifying a positive value will ensure that
> occurrence of the symbol in kallsyms for the patched object will be used
> for patching if it is valid. For external relocations sympos is not
> supported.
>
> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
> as they are no longer used.
>
> >From the klp_reloc structure remove val, as it can be refactored as a
> local variable in klp_write_object_relocations.
>
> Signed-off-by: Chris J Arges <[email protected]>
FWIW
Reviewed-by: Miroslav Benes <[email protected]>
Miroslav