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 <j...@redhat.com>
> ---
> 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().
> #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.
> @@ -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.
> + */
> + 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-api" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
