From 574732c73d155320f9358d9ee5d84beb0f4ecee2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 23 Dec 2014 15:05:36 +1030 Subject: param: initialize store function to NULL if not available. I rebased Kees' 'param: do not set store func without write perm' on top of my 'params: cleanup sysfs allocation'. However, my patch uses krealloc which doesn't zero memory, leaving .store unset. Reported-by: Sasha Levin Cc: Kees Cook Signed-off-by: Rusty Russell diff --git a/kernel/params.c b/kernel/params.c index 0af9b2c..bd65d136 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -648,6 +648,8 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, /* Do not allow runtime DAC changes to make param writable. */ if ((kp->perm & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0) mk->mp->attrs[mk->mp->num].mattr.store = param_attr_store; + else + mk->mp->attrs[mk->mp->num].mattr.store = NULL; mk->mp->attrs[mk->mp->num].mattr.attr.name = (char *)name; mk->mp->attrs[mk->mp->num].mattr.attr.mode = kp->perm; mk->mp->num++; -- cgit v0.10.2 From c772be52319de9756fd82f36d37a6d3e003441e3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 20 Jan 2015 09:07:04 +1030 Subject: param: fix uninitialized read with CONFIG_DEBUG_LOCK_ALLOC ignore_lockdep is uninitialized, and sysfs_attr_init() doesn't initialize it, so memset to 0. Reported-by: Huang Ying Cc: Eric W. Biederman Signed-off-by: Rusty Russell diff --git a/kernel/params.c b/kernel/params.c index bd65d136..728e05b 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -642,6 +642,7 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, mk->mp->grp.attrs = new_attrs; /* Tack new one on the end. */ + memset(&mk->mp->attrs[mk->mp->num], 0, sizeof(mk->mp->attrs[0])); sysfs_attr_init(&mk->mp->attrs[mk->mp->num].mattr.attr); mk->mp->attrs[mk->mp->num].param = kp; mk->mp->attrs[mk->mp->num].mattr.show = param_attr_show; -- cgit v0.10.2 From d453cded05ee219b77815ea194dc36efa5398bca Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 20 Jan 2015 09:07:04 +1030 Subject: module_arch_freeing_init(): new hook for archs before module->module_init freed. Archs have been abusing module_free() to clean up their arch-specific allocations. Since module_free() is also (ab)used by BPF and trace code, let's keep it to simple allocations, and provide a hook called before that. This means that avr32, ia64, parisc and s390 no longer need to implement their own module_free() at all. avr32 doesn't need module_finalize() either. Signed-off-by: Rusty Russell Cc: Chris Metcalf Cc: Haavard Skinnemoen Cc: Hans-Christian Egtvedt Cc: Tony Luck Cc: Fenghua Yu Cc: "James E.J. Bottomley" Cc: Helge Deller Cc: Martin Schwidefsky Cc: Heiko Carstens Cc: linux-kernel@vger.kernel.org Cc: linux-ia64@vger.kernel.org Cc: linux-parisc@vger.kernel.org Cc: linux-s390@vger.kernel.org diff --git a/arch/avr32/kernel/module.c b/arch/avr32/kernel/module.c index 2c94129..164efa0 100644 --- a/arch/avr32/kernel/module.c +++ b/arch/avr32/kernel/module.c @@ -19,12 +19,10 @@ #include #include -void module_free(struct module *mod, void *module_region) +void module_arch_freeing_init(struct module *mod) { vfree(mod->arch.syminfo); mod->arch.syminfo = NULL; - - vfree(module_region); } static inline int check_rela(Elf32_Rela *rela, struct module *module, @@ -291,12 +289,3 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab, return ret; } - -int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, - struct module *module) -{ - vfree(module->arch.syminfo); - module->arch.syminfo = NULL; - - return 0; -} diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c index 24603be..29754aa 100644 --- a/arch/ia64/kernel/module.c +++ b/arch/ia64/kernel/module.c @@ -305,14 +305,12 @@ plt_target (struct plt_entry *plt) #endif /* !USE_BRL */ void -module_free (struct module *mod, void *module_region) +module_arch_freeing_init (struct module *mod) { - if (mod && mod->arch.init_unw_table && - module_region == mod->module_init) { + if (mod->arch.init_unw_table) { unw_remove_unwind_table(mod->arch.init_unw_table); mod->arch.init_unw_table = NULL; } - vfree(module_region); } /* Have we already seen one of these relocations? */ diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c index 50dfafc..5822e8e 100644 --- a/arch/parisc/kernel/module.c +++ b/arch/parisc/kernel/module.c @@ -298,14 +298,10 @@ static inline unsigned long count_stubs(const Elf_Rela *rela, unsigned long n) } #endif - -/* Free memory returned from module_alloc */ -void module_free(struct module *mod, void *module_region) +void module_arch_freeing_init(struct module *mod) { kfree(mod->arch.section); mod->arch.section = NULL; - - vfree(module_region); } /* Additional bytes needed in front of individual sections */ diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c index b89b591..409d152 100644 --- a/arch/s390/kernel/module.c +++ b/arch/s390/kernel/module.c @@ -55,14 +55,10 @@ void *module_alloc(unsigned long size) } #endif -/* Free memory returned from module_alloc */ -void module_free(struct module *mod, void *module_region) +void module_arch_freeing_init(struct module *mod) { - if (mod) { - vfree(mod->arch.syminfo); - mod->arch.syminfo = NULL; - } - vfree(module_region); + vfree(mod->arch.syminfo); + mod->arch.syminfo = NULL; } static void check_rela(Elf_Rela *rela, struct module *me) diff --git a/arch/tile/kernel/module.c b/arch/tile/kernel/module.c index 96447c9..62a597e 100644 --- a/arch/tile/kernel/module.c +++ b/arch/tile/kernel/module.c @@ -83,7 +83,7 @@ void module_free(struct module *mod, void *module_region) 0, 0, 0, NULL, NULL, 0); /* - * FIXME: If module_region == mod->module_init, trim exception + * FIXME: Add module_arch_freeing_init to trim exception * table entries. */ } diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h index 7eeb9bb..054eac8 100644 --- a/include/linux/moduleloader.h +++ b/include/linux/moduleloader.h @@ -82,4 +82,6 @@ int module_finalize(const Elf_Ehdr *hdr, /* Any cleanup needed when module leaves. */ void module_arch_cleanup(struct module *mod); +/* Any cleanup before freeing mod->module_init */ +void module_arch_freeing_init(struct module *mod); #endif diff --git a/kernel/module.c b/kernel/module.c index 3965511..68be0b1f 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1804,6 +1804,10 @@ void __weak module_arch_cleanup(struct module *mod) { } +void __weak module_arch_freeing_init(struct module *mod) +{ +} + /* Free a module, remove from lists, etc. */ static void free_module(struct module *mod) { @@ -1841,6 +1845,7 @@ static void free_module(struct module *mod) /* This may be NULL, but that's OK */ unset_module_init_ro_nx(mod); + module_arch_freeing_init(mod); module_free(mod, mod->module_init); kfree(mod->args); percpu_modfree(mod); @@ -2930,6 +2935,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags) static void module_deallocate(struct module *mod, struct load_info *info) { percpu_modfree(mod); + module_arch_freeing_init(mod); module_free(mod, mod->module_init); module_free(mod, mod->module_core); } @@ -3055,6 +3061,7 @@ static int do_init_module(struct module *mod) mod->strtab = mod->core_strtab; #endif unset_module_init_ro_nx(mod); + module_arch_freeing_init(mod); module_free(mod, mod->module_init); mod->module_init = NULL; mod->init_size = 0; -- cgit v0.10.2 From be1f221c0445a4157d177197c236f888d3581914 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 20 Jan 2015 09:07:05 +1030 Subject: module: remove mod arg from module_free, rename module_memfree(). Nothing needs the module pointer any more, and the next patch will call it from RCU, where the module itself might no longer exist. Removing the arg is the safest approach. This just codifies the use of the module_alloc/module_free pattern which ftrace and bpf use. Signed-off-by: Rusty Russell Acked-by: Alexei Starovoitov Cc: Mikael Starvik Cc: Jesper Nilsson Cc: Ralf Baechle Cc: Ley Foon Tan Cc: Benjamin Herrenschmidt Cc: Chris Metcalf Cc: Steven Rostedt Cc: x86@kernel.org Cc: Ananth N Mavinakayanahalli Cc: Anil S Keshavamurthy Cc: Masami Hiramatsu Cc: linux-cris-kernel@axis.com Cc: linux-kernel@vger.kernel.org Cc: linux-mips@linux-mips.org Cc: nios2-dev@lists.rocketboards.org Cc: linuxppc-dev@lists.ozlabs.org Cc: sparclinux@vger.kernel.org Cc: netdev@vger.kernel.org diff --git a/arch/cris/kernel/module.c b/arch/cris/kernel/module.c index 51123f9..af04cb6 100644 --- a/arch/cris/kernel/module.c +++ b/arch/cris/kernel/module.c @@ -36,7 +36,7 @@ void *module_alloc(unsigned long size) } /* Free memory returned from module_alloc */ -void module_free(struct module *mod, void *module_region) +void module_memfree(void *module_region) { kfree(module_region); } diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c index 9fd6834..5d61393 100644 --- a/arch/mips/net/bpf_jit.c +++ b/arch/mips/net/bpf_jit.c @@ -1388,7 +1388,7 @@ out: void bpf_jit_free(struct bpf_prog *fp) { if (fp->jited) - module_free(NULL, fp->bpf_func); + module_memfree(fp->bpf_func); bpf_prog_unlock_free(fp); } diff --git a/arch/nios2/kernel/module.c b/arch/nios2/kernel/module.c index cc924a3..e2e3f13 100644 --- a/arch/nios2/kernel/module.c +++ b/arch/nios2/kernel/module.c @@ -36,7 +36,7 @@ void *module_alloc(unsigned long size) } /* Free memory returned from module_alloc */ -void module_free(struct module *mod, void *module_region) +void module_memfree(void *module_region) { kfree(module_region); } diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c index 1ca125b..d1916b5 100644 --- a/arch/powerpc/net/bpf_jit_comp.c +++ b/arch/powerpc/net/bpf_jit_comp.c @@ -699,7 +699,7 @@ out: void bpf_jit_free(struct bpf_prog *fp) { if (fp->jited) - module_free(NULL, fp->bpf_func); + module_memfree(fp->bpf_func); bpf_prog_unlock_free(fp); } diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c index f33e7c7..7931eee 100644 --- a/arch/sparc/net/bpf_jit_comp.c +++ b/arch/sparc/net/bpf_jit_comp.c @@ -776,7 +776,7 @@ cond_branch: f_offset = addrs[i + filter[i].jf]; if (unlikely(proglen + ilen > oldproglen)) { pr_err("bpb_jit_compile fatal error\n"); kfree(addrs); - module_free(NULL, image); + module_memfree(image); return; } memcpy(image + proglen, temp, ilen); @@ -822,7 +822,7 @@ out: void bpf_jit_free(struct bpf_prog *fp) { if (fp->jited) - module_free(NULL, fp->bpf_func); + module_memfree(fp->bpf_func); bpf_prog_unlock_free(fp); } diff --git a/arch/tile/kernel/module.c b/arch/tile/kernel/module.c index 62a597e..2305084 100644 --- a/arch/tile/kernel/module.c +++ b/arch/tile/kernel/module.c @@ -74,7 +74,7 @@ error: /* Free memory returned from module_alloc */ -void module_free(struct module *mod, void *module_region) +void module_memfree(void *module_region) { vfree(module_region); diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 2142376..8b7b0a5 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -674,7 +674,7 @@ static inline void *alloc_tramp(unsigned long size) } static inline void tramp_free(void *tramp) { - module_free(NULL, tramp); + module_memfree(tramp); } #else /* Trampolines can only be created if modules are supported */ diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h index 054eac8..f755626 100644 --- a/include/linux/moduleloader.h +++ b/include/linux/moduleloader.h @@ -26,7 +26,7 @@ unsigned int arch_mod_section_prepend(struct module *mod, unsigned int section); void *module_alloc(unsigned long size); /* Free memory returned from module_alloc. */ -void module_free(struct module *mod, void *module_region); +void module_memfree(void *module_region); /* * Apply the given relocation to the (simplified) ELF. Return -error diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index d6594e4..a64e7a2 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -163,7 +163,7 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, void bpf_jit_binary_free(struct bpf_binary_header *hdr) { - module_free(NULL, hdr); + module_memfree(hdr); } #endif /* CONFIG_BPF_JIT */ diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 06f5830..ee61992 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -127,7 +127,7 @@ static void *alloc_insn_page(void) static void free_insn_page(void *page) { - module_free(NULL, page); + module_memfree(page); } struct kprobe_insn_cache kprobe_insn_slots = { diff --git a/kernel/module.c b/kernel/module.c index 68be0b1f..1f85fd5 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1795,7 +1795,7 @@ static void unset_module_core_ro_nx(struct module *mod) { } static void unset_module_init_ro_nx(struct module *mod) { } #endif -void __weak module_free(struct module *mod, void *module_region) +void __weak module_memfree(void *module_region) { vfree(module_region); } @@ -1846,7 +1846,7 @@ static void free_module(struct module *mod) /* This may be NULL, but that's OK */ unset_module_init_ro_nx(mod); module_arch_freeing_init(mod); - module_free(mod, mod->module_init); + module_memfree(mod->module_init); kfree(mod->args); percpu_modfree(mod); @@ -1855,7 +1855,7 @@ static void free_module(struct module *mod) /* Finally, free the core (containing the module structure) */ unset_module_core_ro_nx(mod); - module_free(mod, mod->module_core); + module_memfree(mod->module_core); #ifdef CONFIG_MPU update_protections(current->mm); @@ -2790,7 +2790,7 @@ static int move_module(struct module *mod, struct load_info *info) */ kmemleak_ignore(ptr); if (!ptr) { - module_free(mod, mod->module_core); + module_memfree(mod->module_core); return -ENOMEM; } memset(ptr, 0, mod->init_size); @@ -2936,8 +2936,8 @@ static void module_deallocate(struct module *mod, struct load_info *info) { percpu_modfree(mod); module_arch_freeing_init(mod); - module_free(mod, mod->module_init); - module_free(mod, mod->module_core); + module_memfree(mod->module_init); + module_memfree(mod->module_core); } int __weak module_finalize(const Elf_Ehdr *hdr, @@ -3062,7 +3062,7 @@ static int do_init_module(struct module *mod) #endif unset_module_init_ro_nx(mod); module_arch_freeing_init(mod); - module_free(mod, mod->module_init); + module_memfree(mod->module_init); mod->module_init = NULL; mod->init_size = 0; mod->init_ro_size = 0; -- cgit v0.10.2 From c749637909eea5d4090c6f50b89c2c20b534a280 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 20 Jan 2015 09:07:05 +1030 Subject: module: fix race in kallsyms resolution during module load success. The kallsyms routines (module_symbol_name, lookup_module_* etc) disable preemption to walk the modules rather than taking the module_mutex: this is because they are used for symbol resolution during oopses. This works because there are synchronize_sched() and synchronize_rcu() in the unload and failure paths. However, there's one case which doesn't have that: the normal case where module loading succeeds, and we free the init section. We don't want a synchronize_rcu() there, because it would slow down module loading: this bug was introduced in 2009 to speed module loading in the first place. Thus, we want to do the free in an RCU callback. We do this in the simplest possible way by allocating a new rcu_head: if we put it in the module structure we'd have to worry about that getting freed. Reported-by: Rui Xiang Signed-off-by: Rusty Russell diff --git a/kernel/module.c b/kernel/module.c index 1f85fd5..ed4ec9c 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2989,10 +2989,31 @@ static void do_mod_ctors(struct module *mod) #endif } +/* For freeing module_init on success, in case kallsyms traversing */ +struct mod_initfree { + struct rcu_head rcu; + void *module_init; +}; + +static void do_free_init(struct rcu_head *head) +{ + struct mod_initfree *m = container_of(head, struct mod_initfree, rcu); + module_memfree(m->module_init); + kfree(m); +} + /* This is where the real work happens */ static int do_init_module(struct module *mod) { int ret = 0; + struct mod_initfree *freeinit; + + freeinit = kmalloc(sizeof(*freeinit), GFP_KERNEL); + if (!freeinit) { + ret = -ENOMEM; + goto fail; + } + freeinit->module_init = mod->module_init; /* * We want to find out whether @mod uses async during init. Clear @@ -3005,18 +3026,7 @@ static int do_init_module(struct module *mod) if (mod->init != NULL) ret = do_one_initcall(mod->init); if (ret < 0) { - /* - * Init routine failed: abort. Try to protect us from - * buggy refcounters. - */ - mod->state = MODULE_STATE_GOING; - synchronize_sched(); - module_put(mod); - blocking_notifier_call_chain(&module_notify_list, - MODULE_STATE_GOING, mod); - free_module(mod); - wake_up_all(&module_wq); - return ret; + goto fail_free_freeinit; } if (ret > 0) { pr_warn("%s: '%s'->init suspiciously returned %d, it should " @@ -3062,15 +3072,34 @@ static int do_init_module(struct module *mod) #endif unset_module_init_ro_nx(mod); module_arch_freeing_init(mod); - module_memfree(mod->module_init); mod->module_init = NULL; mod->init_size = 0; mod->init_ro_size = 0; mod->init_text_size = 0; + /* + * We want to free module_init, but be aware that kallsyms may be + * walking this with preempt disabled. In all the failure paths, + * we call synchronize_rcu/synchronize_sched, but we don't want + * to slow down the success path, so use actual RCU here. + */ + call_rcu(&freeinit->rcu, do_free_init); mutex_unlock(&module_mutex); wake_up_all(&module_wq); return 0; + +fail_free_freeinit: + kfree(freeinit); +fail: + /* Try to protect us from buggy refcounters. */ + mod->state = MODULE_STATE_GOING; + synchronize_sched(); + module_put(mod); + blocking_notifier_call_chain(&module_notify_list, + MODULE_STATE_GOING, mod); + free_module(mod); + wake_up_all(&module_wq); + return ret; } static int may_init_module(void) -- cgit v0.10.2 From d5db139ab3764640e0882a1746e7b9fdee33fd87 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 22 Jan 2015 11:13:14 +1030 Subject: module: make module_refcount() a signed integer. James Bottomley points out that it will be -1 during unload. It's only used for diagnostics, so let's not hide that as it could be a clue as to what's gone wrong. Cc: Jason Wessel Acked-and-documention-added-by: James Bottomley Reviewed-by: Masami Hiramatsu Signed-off-by: Rusty Russell diff --git a/include/linux/module.h b/include/linux/module.h index ebfb0e1..b653d7c 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -444,7 +444,7 @@ extern void __module_put_and_exit(struct module *mod, long code) #define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE, code) #ifdef CONFIG_MODULE_UNLOAD -unsigned long module_refcount(struct module *mod); +int module_refcount(struct module *mod); void __symbol_put(const char *symbol); #define symbol_put(x) __symbol_put(VMLINUX_SYMBOL_STR(x)) void symbol_put_addr(void *addr); diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 379650b..2934889 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -1979,7 +1979,7 @@ static int kdb_lsmod(int argc, const char **argv) kdb_printf("%-20s%8u 0x%p ", mod->name, mod->core_size, (void *)mod); #ifdef CONFIG_MODULE_UNLOAD - kdb_printf("%4ld ", module_refcount(mod)); + kdb_printf("%4d ", module_refcount(mod)); #endif if (mod->state == MODULE_STATE_GOING) kdb_printf(" (Unloading)"); diff --git a/kernel/module.c b/kernel/module.c index ed4ec9c..d856e96 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -772,9 +772,18 @@ static int try_stop_module(struct module *mod, int flags, int *forced) return 0; } -unsigned long module_refcount(struct module *mod) +/** + * module_refcount - return the refcount or -1 if unloading + * + * @mod: the module we're checking + * + * Returns: + * -1 if the module is in the process of unloading + * otherwise the number of references in the kernel to the module + */ +int module_refcount(struct module *mod) { - return (unsigned long)atomic_read(&mod->refcnt) - MODULE_REF_BASE; + return atomic_read(&mod->refcnt) - MODULE_REF_BASE; } EXPORT_SYMBOL(module_refcount); @@ -856,7 +865,7 @@ static inline void print_unload_info(struct seq_file *m, struct module *mod) struct module_use *use; int printed_something = 0; - seq_printf(m, " %lu ", module_refcount(mod)); + seq_printf(m, " %i ", module_refcount(mod)); /* * Always include a trailing , so userspace can differentiate @@ -908,7 +917,7 @@ EXPORT_SYMBOL_GPL(symbol_put_addr); static ssize_t show_refcnt(struct module_attribute *mattr, struct module_kobject *mk, char *buffer) { - return sprintf(buffer, "%lu\n", module_refcount(mk->mod)); + return sprintf(buffer, "%i\n", module_refcount(mk->mod)); } static struct module_attribute modinfo_refcnt = -- cgit v0.10.2