Re: module: preserve Elf information for livepatch modules

From: Jessica Yu
Date: Wed Dec 09 2015 - 15:05:34 EST

+++ Josh Poimboeuf [08/12/15 12:32 -0600]:
On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
---
include/linux/module.h | 9 +++++
kernel/module.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {

/* Notes attributes */
struct module_notes_attrs *notes_attrs;
+
+ /* Elf information (optionally saved) */
+ Elf_Ehdr *hdr;

I would rename "hdr" to "elf_hdr" to make its purpose clearer.

+ Elf_Shdr *sechdrs;
+ char *secstrings;

Probably a good idea to add underscores to the names ("sec_hdrs" and
"sec_strings") to be consistent with most of the other fields in the
struct.

+ struct {
+ unsigned int sym, str, mod, vers, info, pcpu;
+ } index;

I might be contradicting myself from what I said before. But I'm
thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
Then below, there could be two versions of copy_module_elf(), the real
one for LIVEPATCH and and an empty one for !LIVEPATCH. And the same
story for free_module_elf().

I think in the v1 discussion we were leaning more towards making this
generic. We could potentially just have the Elf module fields
available in the generic case, independent of whether CONFIG_LIVEPATCH
is set, whereas the mod->klp field should probably be only available
when LIVEPATCH is set. I think this makes sense since the Elf fields
aren't dependent on livepatch (although livepatch would be the only
user of these fields at the moment). I don't know if there would be
any users in the future that would be interested in using this Elf
information. Thoughts on this?

#endif

/* The command line arguments (may be mangled). People like
@@ -461,6 +469,7 @@ struct module {
#endif

#ifdef CONFIG_LIVEPATCH
+ bool klp; /* Is this a livepatch module? */
bool klp_alive;
#endif

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) { }
static void unset_module_init_ro_nx(struct module *mod) { }
#endif

+static void free_module_elf(struct module *mod)
+{
+ kfree(mod->hdr);
+ kfree(mod->sechdrs);
+ kfree(mod->secstrings);
+}
+
void __weak module_memfree(void *module_region)
{
vfree(module_region);
@@ -2022,6 +2029,9 @@ static void free_module(struct module *mod)
/* Free any allocated parameters. */
destroy_params(mod->kp, mod->num_kp);

+ /* Free Elf information if it was saved */
+ free_module_elf(mod);
+
/* Now we can delete it from the lists */
mutex_lock(&module_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
(long)sym[i].st_value);
break;

+ case SHN_LIVEPATCH:
+ /* klp symbols are resolved by livepatch */
+ break;
+
case SHN_UNDEF:
ksym = resolve_symbol_wait(mod, info, name);
/* Ok if resolved. */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;

+ /* klp relocation sections are applied by livepatch */
+ if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+ continue;
+
if (info->sechdrs[i].sh_type == SHT_REL)
err = apply_relocate(info->sechdrs, info->strtab,
info->index.sym, i, mod);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
{
const Elf_Shdr *sechdrs = info->sechdrs;

+ if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+ return 'K';
+ if (sym->st_shndx == SHN_LIVEPATCH)
+ return 'k';
+
if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
return 'v';
@@ -2475,7 +2498,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)

/* Compute total space required for the core symbols' strtab. */
for (ndst = i = 0; i < nsrc; i++) {
- if (i == 0 ||
+ if (i == 0 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
strtab_size += strlen(&info->strtab[src[i].st_name])+1;
ndst++;

Instead of accessing mod->klp directly, how about an
'is_livepatch_module(mod)' function. There could be two versions, with
the !LIVEPATCH version always returning false and the LIVEPATCH version
checking mod->klp. Then mod->klp itself can stay inside the LIVEPATCH
ifdef in the module struct.

OK, that sounds good. Makes more sense for the klp field to exist only
in the LIVEPATCH case anyway.

@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
mod->core_strtab = s = mod->module_core + info->stroffs;
src = mod->symtab;
for (ndst = i = 0; i < mod->num_symtab; i++) {
- if (i == 0 ||
+ if (i == 0 || mod->klp ||
is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
dst[ndst] = src[i];
dst[ndst++].st_name = s - mod->core_strtab;
@@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
return 0;
}

+/*
+ * copy_module_elf - preserve Elf information about a module
+ */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+ unsigned int size;
+ int ret = 0;

No need to initialize ret to zero here since it's never used
uninitalized.

+ Elf_Shdr *symsect;
+
+ /* Elf header */
+ size = sizeof(Elf_Ehdr);
+ mod->hdr = kzalloc(size, GFP_KERNEL);

No need to zero the memory here with kzalloc() since it will all be
memcpy()'d anyway. kmalloc() can be used instead (and the same for the
other kzalloc()s below).

+ if (mod->hdr == NULL) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ memcpy(mod->hdr, info->hdr, size);
+
+ /* Elf section header table */
+ size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
+ mod->sechdrs = kzalloc(size, GFP_KERNEL);
+ if (mod->sechdrs == NULL) {
+ ret = -ENOMEM;
+ goto free_hdr;
+ }
+ memcpy(mod->sechdrs, info->sechdrs, size);
+
+ /* Elf section name string table */
+ size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
+ mod->secstrings = kzalloc(size, GFP_KERNEL);
+ if (mod->secstrings == NULL) {
+ ret = -ENOMEM;
+ goto free_sechdrs;
+ }
+ memcpy(mod->secstrings, info->secstrings, size);
+
+ /* Elf section indices */
+ memcpy(&mod->index, &info->index, sizeof(info->index));
+
+ /*
+ * Update symtab's sh_addr to point to a valid
+ * symbol table, as the temporary symtab in module
+ * init memory will be freed
+ */
+ symsect = mod->sechdrs + mod->index.sym;
+ symsect->sh_addr = (unsigned long)mod->core_symtab;
+
+ return ret;
+
+free_sechdrs:
+ kfree(mod->sechdrs);
+free_hdr:
+ kfree(mod->hdr);
+out:
+ return ret;
+}
+
+
#define COPY_CHUNK_SIZE (16*PAGE_SIZE)

static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned long len)
@@ -2866,6 +2947,9 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
"is unknown, you have been warned.\n", mod->name);
}

+ if (get_modinfo(info, "livepatch"))
+ mod->klp = true;
+

Similar to the is_livepatch_module() function I suggested, this can be
put in a function so that mod->klp can be abstracted away for the
!LIVEPATCH case. Maybe there should be a check_livepatch_modinfo()
function:

1. the !LIVEPATCH version of the function could return an error if
modinfo has "livepatch"

2. the LIVEPATCH version could simply set mod->klp = true.

/* Set up license info based on the info section */
set_license(mod, get_modinfo(info, "license"));

@@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
if (err < 0)
goto bug_cleanup;

+ /*
+ * Save sechdrs, indices, and other data from info
+ * in order to patch to-be-loaded modules.
+ * Do not call free_copy() for livepatch modules.

I think the last line of this comment isn't right, since free_copy() is
called below regardless.

Yes you're right, forgot to update that comment.

+ */
+ if (mod->klp)
+ err = copy_module_elf(mod, info);
+ if (err < 0)
+ goto bug_cleanup;

Not strictly necessary, but I think it would be a little cleaner to only
check the err if copy_module_elf() was called.

+
/* Get rid of temporary copy. */
free_copy(info);

--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/