| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 Nov 2015 15:30:53 +0100 (CET)
From: Miroslav Benes <mbenes@...e.cz>
To: Jessica Yu <jeyu@...hat.com>
cc: Rusty Russell <rusty@...tcorp.com.au>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Seth Jennings <sjenning@...hat.com>,
Jiri Kosina <jikos@...nel.org>,
Vojtech Pavlik <vojtech@...e.com>, linux-api@...r.kernel.org,
live-patching@...r.kernel.org, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write
relocations
On Mon, 9 Nov 2015, Jessica Yu wrote:
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..601e892 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -85,7 +85,7 @@ struct klp_reloc {
> /**
> * struct klp_object - kernel object structure for live patching
> * @name: module name (or NULL for vmlinux)
> - * @relocs: relocation entries to be applied at load time
> + * @reloc_secs: relocation sections to be applied at load time
> * @funcs: function entries for functions to be patched in the object
> * @kobj: kobject for sysfs resources
> * @mod: kernel module associated with the patched object
> @@ -95,7 +95,7 @@ struct klp_reloc {
> struct klp_object {
> /* external */
> const char *name;
> - struct klp_reloc *relocs;
> + struct list_head reloc_secs;
> struct klp_func *funcs;
So I guess we don't need klp_reloc anymore. If true, we should really
start thinking about proper documentation because there are going to be
plenty of assumptions about a patch module and we need to have it written
somewhere. Especially how the relocation sections look like.
> /* internal */
> @@ -129,6 +129,13 @@ struct klp_patch {
> #define klp_for_each_func(obj, func) \
> for (func = obj->funcs; func->old_name; func++)
>
> +struct klp_reloc_sec {
> + unsigned int index;
> + char *name;
> + char *objname;
> + struct list_head list;
> +};
Description of the structure and its members is missing.
> diff --git a/include/linux/module.h b/include/linux/module.h
> index c8680b1..3c34eb8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -793,9 +793,15 @@ extern int module_sysfs_initialized;
> #ifdef CONFIG_DEBUG_SET_MODULE_RONX
> extern void set_all_modules_text_rw(void);
> extern void set_all_modules_text_ro(void);
> +extern void
> +set_page_attributes(void *start, void *end,
> + int (*set)(unsigned long start, int num_pages));
> #else
> static inline void set_all_modules_text_rw(void) { }
> static inline void set_all_modules_text_ro(void) { }
> +static inline void
> +set_page_attributes(void *start, void *end,
> + int (*set)(unsigned long start, int num_pages)) { }
> #endif
This would be solved after Rusty's and Josh's patches get merged, right?
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 087a8c7..26c419f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,8 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
> +#include <linux/elf.h>
> +#include <asm/cacheflush.h>
>
> /**
> * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -281,46 +283,54 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
> }
>
> static int klp_write_object_relocations(struct module *pmod,
> - struct klp_object *obj)
> + struct klp_object *obj,
> + struct klp_patch *patch)
> {
> - int ret;
> - struct klp_reloc *reloc;
> + int relindex, num_relas;
> + int i, ret = 0;
> + unsigned long addr;
> + unsigned int bind;
> + char *symname;
> + struct klp_reloc_sec *reloc_sec;
> + struct load_info *info;
> + Elf_Rela *rela;
> + Elf_Sym *sym, *symtab;
> + Elf_Shdr *symsect;
>
> if (WARN_ON(!klp_is_object_loaded(obj)))
> return -EINVAL;
>
> - if (WARN_ON(!obj->relocs))
> - 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,
> - &reloc->val);
> - if (ret)
> - return ret;
> - }
> - ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> - reloc->val + reloc->addend);
> - if (ret) {
> - pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> - reloc->name, reloc->val, ret);
> - return ret;
> + info = pmod->info;
> + symsect = info->sechdrs + info->index.sym;
> + symtab = (void *)info->hdr + symsect->sh_offset;
> +
> + /* For each __klp_rela section for this object */
> + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
> + relindex = reloc_sec->index;
> + num_relas = info->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> + rela = (Elf_Rela *) info->sechdrs[relindex].sh_addr;
> +
> + /* For each rela in this __klp_rela section */
> + for (i = 0; i < num_relas; i++, rela++) {
> + sym = symtab + ELF_R_SYM(rela->r_info);
> + symname = info->strtab + sym->st_name;
> + bind = ELF_ST_BIND(sym->st_info);
> +
> + if (sym->st_shndx == SHN_LIVEPATCH) {
> + if (bind == STB_LIVEPATCH_EXT)
> + ret = klp_find_external_symbol(pmod, symname, &addr);
> + else
> + ret = klp_find_object_symbol(obj->name, symname, &addr);
> + if (ret)
> + return ret;
> + sym->st_value = addr;
> + }
> }
> + ret = apply_relocate_add(info->sechdrs, info->strtab,
> + info->index.sym, relindex, pmod);
> }
>
> - return 0;
> + return ret;
> }
Looking at this... do we even need reloc_secs in klp_object? Question is
whether we need more than one dynrela section for an object. If not then
the binding between klp_reloc_sec and an object is the only relevant thing
in the structure, be it index or objname. So we can replace the
list of structures with just the index in klp_object, or get rid of it
completely and rely on the name of dynrela section be something like
__klp_rela_{objname}.
You see, we go through elf sections here which were preserved by module
loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
maybe all the stuff around klp_reloc_sec is not necessary.
Thoughts?
> @@ -741,12 +751,23 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> struct klp_object *obj)
> {
> struct klp_func *func;
> + struct module *pmod;
> int ret;
>
> - if (obj->relocs) {
> - ret = klp_write_object_relocations(patch->mod, obj);
> + pmod = patch->mod;
> +
> + if (!list_empty(&obj->reloc_secs)) {
> + set_page_attributes(pmod->module_core,
> + pmod->module_core + pmod->core_text_size,
> + set_memory_rw);
> +
> + ret = klp_write_object_relocations(pmod, obj, patch);
> if (ret)
> return ret;
> +
> + set_page_attributes(pmod->module_core,
> + pmod->module_core + pmod->core_text_size,
> + set_memory_ro);
> }
And this would get solved with different patches as well. I think the
calls to set_page_attributes() should be hidden in
klp_write_object_relocations() as it is in Josh's patch IIRC.
Thanks,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists