| Message ID | 20191119155149.20396-1-tli@digitalocean.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] kernel/module: have the callers of set_memory_*() check the return value | expand |
On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: > Right now several architectures allow their set_memory_*() family of > functions to fail, but callers may not be checking the return values. We > need to fix the callers and add the __must_check attribute. They also may > not provide any level of atomicity, in the sense that the memory > protections may be left incomplete on failure. This issue likely has a few > steps on effects architectures[1]: Awesome; thanks for working on this! A few suggestions on this patch, which will help reviewers, below... > 1)Have all callers of set_memory_*() helpers check the return value. > 2)Add __much_check to all set_memory_*() helpers so that new uses do not > ignore the return value. > 3)Add atomicity to the calls so that the memory protections aren't left in > a partial state. Maybe clarify to say something like "this series is step 1"? > > Ideally, the failure of set_memory_*() should be passed up the call stack, > and callers should examine the failure and deal with it. But currently, > some callers just have void return type. > > We need to fix the callers to handle the return all the way to the top of > stack, and it will require a large series of patches to finish all the three > steps mentioned above. I start with kernel/module, and will move onto other > subsystems. I am not entirely sure about the failure modes for each caller. > So I would like to get some comments before I move forward. This single > patch is just for fixing the return value of set_memory_*() function in > kernel/module, and also the related callers. Any feedback would be greatly > appreciated. > > [1]:https://github.com/KSPP/linux/issues/7 > > Signed-off-by: Tianlin Li <tli@digitalocean.com> > --- > arch/arm/kernel/ftrace.c | 8 +- > arch/arm64/kernel/ftrace.c | 6 +- > arch/nds32/kernel/ftrace.c | 6 +- > arch/x86/kernel/ftrace.c | 13 ++- > include/linux/module.h | 16 ++-- > kernel/livepatch/core.c | 15 +++- > kernel/module.c | 170 +++++++++++++++++++++++++++---------- > kernel/trace/ftrace.c | 15 +++- > 8 files changed, 175 insertions(+), 74 deletions(-) - Can you break this patch into 3 separate patches, by "subsystem": - ftrace - module - livepatch (unless Jessica thinks maybe livepatch should go with module?) - Please run patches through scripts/checkpatch.pl to catch common coding style issues (e.g. blank line between variable declarations and function body). - These changes look pretty straight forward, so I think it'd be fine to drop the "RFC" tag and include linux-kernel@vger.kernel.org in the Cc list for the v2. For lots of detail, see: https://www.kernel.org/doc/html/latest/process/submitting-patches.html More below... > > diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c > index bda949fd84e8..7ea1338821d6 100644 > --- a/arch/arm/kernel/ftrace.c > +++ b/arch/arm/kernel/ftrace.c > @@ -59,13 +59,15 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr) > > int ftrace_arch_code_modify_prepare(void) > { > - set_all_modules_text_rw(); > - return 0; > + return set_all_modules_text_rw(); > } > > int ftrace_arch_code_modify_post_process(void) > { > - set_all_modules_text_ro(); > + int ret; Blank line here... > + ret = set_all_modules_text_ro(); > + if (ret) > + return ret; > /* Make sure any TLB misses during machine stop are cleared. */ > flush_tlb_all(); > return 0; Are callers of these ftrace functions checking return values too? > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c > index 171773257974..97a89c38f6b9 100644 > --- a/arch/arm64/kernel/ftrace.c > +++ b/arch/arm64/kernel/ftrace.c > @@ -115,9 +115,11 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > } > > /* point the trampoline to our ftrace entry point */ > - module_disable_ro(mod); > + if (module_disable_ro(mod)) > + return -EINVAL; > *dst = trampoline; > - module_enable_ro(mod, true); > + if (module_enable_ro(mod, true)) > + return -EINVAL; > > /* > * Ensure updated trampoline is visible to instruction > diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c > index fd2a54b8cd57..e9e63e703a3e 100644 > --- a/arch/nds32/kernel/ftrace.c > +++ b/arch/nds32/kernel/ftrace.c > @@ -91,14 +91,12 @@ int __init ftrace_dyn_arch_init(void) > > int ftrace_arch_code_modify_prepare(void) > { > - set_all_modules_text_rw(); > - return 0; > + return set_all_modules_text_rw(); > } > > int ftrace_arch_code_modify_post_process(void) > { > - set_all_modules_text_ro(); > - return 0; > + return set_all_modules_text_ro(); > } > > static unsigned long gen_sethi_insn(unsigned long addr) > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index 024c3053dbba..7fdee06e2a76 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -42,19 +42,26 @@ int ftrace_arch_code_modify_prepare(void) > * and live kernel patching from changing the text permissions while > * ftrace has it set to "read/write". > */ > + int ret; > mutex_lock(&text_mutex); > set_kernel_text_rw(); > - set_all_modules_text_rw(); > + ret = set_all_modules_text_rw(); > + if (ret) { > + set_kernel_text_ro(); Is the set_kernel_text_ro() here is an atomicity fix? Also, won't a future patch need to check the result of that function call? (i.e. should it be left out of this series and should atomicity fixes happen inside the set_memory_*() functions? Can they happen there?) > + mutex_unlock(&text_mutex); > + return ret; > + } > return 0; > } > > int ftrace_arch_code_modify_post_process(void) > __releases(&text_mutex) > { > - set_all_modules_text_ro(); > + int ret; blank line needed... > + ret = set_all_modules_text_ro(); > set_kernel_text_ro(); > mutex_unlock(&text_mutex); > - return 0; > + return ret; > } > > union ftrace_code_union { > diff --git a/include/linux/module.h b/include/linux/module.h > index 1455812dd325..e6c7f3b719a3 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -847,15 +847,15 @@ extern int module_sysfs_initialized; > #define __MODULE_STRING(x) __stringify(x) > > #ifdef CONFIG_STRICT_MODULE_RWX > -extern void set_all_modules_text_rw(void); > -extern void set_all_modules_text_ro(void); > -extern void module_enable_ro(const struct module *mod, bool after_init); > -extern void module_disable_ro(const struct module *mod); > +extern int set_all_modules_text_rw(void); > +extern int set_all_modules_text_ro(void); > +extern int module_enable_ro(const struct module *mod, bool after_init); > +extern int module_disable_ro(const struct module *mod); > #else > -static inline void set_all_modules_text_rw(void) { } > -static inline void set_all_modules_text_ro(void) { } > -static inline void module_enable_ro(const struct module *mod, bool after_init) { } > -static inline void module_disable_ro(const struct module *mod) { } > +static inline int set_all_modules_text_rw(void) { return 0; } > +static inline int set_all_modules_text_ro(void) { return 0; } > +static inline int module_enable_ro(const struct module *mod, bool after_init) { return 0; } Please wrap this line (yes, the old one wasn't...) > +static inline int module_disable_ro(const struct module *mod) { return 0; } > #endif > > #ifdef CONFIG_GENERIC_BUG > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index c4ce08f43bd6..39bfc0685854 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct klp_patch *patch, > > mutex_lock(&text_mutex); > > - module_disable_ro(patch->mod); > + ret = module_disable_ro(patch->mod); > + if (ret) { > + mutex_unlock(&text_mutex); > + return ret; > + } > ret = klp_write_object_relocations(patch->mod, obj); > if (ret) { > - module_enable_ro(patch->mod, true); > + if (module_enable_ro(patch->mod, true)); > + pr_err("module_enable_ro failed.\n"); Why the pr_err() here and not in other failure cases? (Also, should it instead be a WARN_ONCE() inside the set_memory_*() functions instead?) > mutex_unlock(&text_mutex); > return ret; > } > > arch_klp_init_object_loaded(patch, obj); > - module_enable_ro(patch->mod, true); > + ret = module_enable_ro(patch->mod, true); > + if (ret) { > + mutex_unlock(&text_mutex); > + return ret; > + } > > mutex_unlock(&text_mutex); > > diff --git a/kernel/module.c b/kernel/module.c > index 9ee93421269c..87125b5e315c 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -1900,111 +1900,162 @@ static void mod_sysfs_teardown(struct module *mod) > * > * These values are always page-aligned (as is base) > */ > -static void frob_text(const struct module_layout *layout, > +static int frob_text(const struct module_layout *layout, > int (*set_memory)(unsigned long start, int num_pages)) > { > BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); > - set_memory((unsigned long)layout->base, > + return set_memory((unsigned long)layout->base, > layout->text_size >> PAGE_SHIFT); > } > > #ifdef CONFIG_STRICT_MODULE_RWX > -static void frob_rodata(const struct module_layout *layout, > +static int frob_rodata(const struct module_layout *layout, > int (*set_memory)(unsigned long start, int num_pages)) > { > BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); > - set_memory((unsigned long)layout->base + layout->text_size, > + return set_memory((unsigned long)layout->base + layout->text_size, > (layout->ro_size - layout->text_size) >> PAGE_SHIFT); > } > > -static void frob_ro_after_init(const struct module_layout *layout, > +static int frob_ro_after_init(const struct module_layout *layout, > int (*set_memory)(unsigned long start, int num_pages)) > { > BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1)); > - set_memory((unsigned long)layout->base + layout->ro_size, > + return set_memory((unsigned long)layout->base + layout->ro_size, > (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT); > } > > -static void frob_writable_data(const struct module_layout *layout, > +static int frob_writable_data(const struct module_layout *layout, > int (*set_memory)(unsigned long start, int num_pages)) > { > BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1)); > BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1)); > - set_memory((unsigned long)layout->base + layout->ro_after_init_size, > + return set_memory((unsigned long)layout->base + layout->ro_after_init_size, > (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT); > } > > /* livepatching wants to disable read-only so it can frob module. */ > -void module_disable_ro(const struct module *mod) > +int module_disable_ro(const struct module *mod) > { > + int ret; > if (!rodata_enabled) > - return; > + return 0; > > - frob_text(&mod->core_layout, set_memory_rw); > - frob_rodata(&mod->core_layout, set_memory_rw); > - frob_ro_after_init(&mod->core_layout, set_memory_rw); > - frob_text(&mod->init_layout, set_memory_rw); > - frob_rodata(&mod->init_layout, set_memory_rw); > + ret = frob_text(&mod->core_layout, set_memory_rw); > + if (ret) > + return ret; > + ret = frob_rodata(&mod->core_layout, set_memory_rw); > + if (ret) > + return ret; > + ret = frob_ro_after_init(&mod->core_layout, set_memory_rw); > + if (ret) > + return ret; > + ret = frob_text(&mod->init_layout, set_memory_rw); > + if (ret) > + return ret; > + ret = frob_rodata(&mod->init_layout, set_memory_rw); > + if (ret) > + return ret; > + > + return 0; > } > > -void module_enable_ro(const struct module *mod, bool after_init) > +int module_enable_ro(const struct module *mod, bool after_init) > { > + int ret; > if (!rodata_enabled) > - return; > + return 0; > > set_vm_flush_reset_perms(mod->core_layout.base); > set_vm_flush_reset_perms(mod->init_layout.base); > - frob_text(&mod->core_layout, set_memory_ro); > + ret = frob_text(&mod->core_layout, set_memory_ro); > + if (ret) > + return ret; > > - frob_rodata(&mod->core_layout, set_memory_ro); > - frob_text(&mod->init_layout, set_memory_ro); > - frob_rodata(&mod->init_layout, set_memory_ro); > + ret = frob_rodata(&mod->core_layout, set_memory_ro); > + if (ret) > + return ret; > + ret = frob_text(&mod->init_layout, set_memory_ro); > + if (ret) > + return ret; > + ret = frob_rodata(&mod->init_layout, set_memory_ro); > + if (ret) > + return ret; > > - if (after_init) > - frob_ro_after_init(&mod->core_layout, set_memory_ro); > + if (after_init) { > + ret = frob_ro_after_init(&mod->core_layout, set_memory_ro); > + if (ret) > + return ret; > + } > + return 0; > } > > -static void module_enable_nx(const struct module *mod) > +static int module_enable_nx(const struct module *mod) > { > - frob_rodata(&mod->core_layout, set_memory_nx); > - frob_ro_after_init(&mod->core_layout, set_memory_nx); > - frob_writable_data(&mod->core_layout, set_memory_nx); > - frob_rodata(&mod->init_layout, set_memory_nx); > - frob_writable_data(&mod->init_layout, set_memory_nx); > + int ret; > + > + ret = frob_rodata(&mod->core_layout, set_memory_nx); > + if (ret) > + return ret; > + ret = frob_ro_after_init(&mod->core_layout, set_memory_nx); > + if (ret) > + return ret; > + ret = frob_writable_data(&mod->core_layout, set_memory_nx); > + if (ret) > + return ret; > + ret = frob_rodata(&mod->init_layout, set_memory_nx); > + if (ret) > + return ret; > + ret = frob_writable_data(&mod->init_layout, set_memory_nx); > + if (ret) > + return ret; > + > + return 0; > } > > /* Iterate through all modules and set each module's text as RW */ > -void set_all_modules_text_rw(void) > +int set_all_modules_text_rw(void) > { > struct module *mod; > + int ret; > > if (!rodata_enabled) > - return; > + return 0; > > mutex_lock(&module_mutex); > list_for_each_entry_rcu(mod, &modules, list) { > if (mod->state == MODULE_STATE_UNFORMED) > continue; > > - frob_text(&mod->core_layout, set_memory_rw); > - frob_text(&mod->init_layout, set_memory_rw); > + ret = frob_text(&mod->core_layout, set_memory_rw); > + if (ret) { > + mutex_unlock(&module_mutex); > + return ret; > + } > + ret = frob_text(&mod->init_layout, set_memory_rw); > + if (ret) { > + mutex_unlock(&module_mutex); > + return ret; > + } This pattern feels like it might be better with a "goto out" style: mutex_lock... list_for_each... { ret = frob_... if (ret) goto out; ret = frob_... if (ret) goto out; } ret = 0; out: mutex_unlock... return ret; > } > mutex_unlock(&module_mutex); > + return 0; > } > > /* Iterate through all modules and set each module's text as RO */ > -void set_all_modules_text_ro(void) > +int set_all_modules_text_ro(void) > { > struct module *mod; > + int ret; > > if (!rodata_enabled) > - return; > + return 0; > > mutex_lock(&module_mutex); > list_for_each_entry_rcu(mod, &modules, list) { > @@ -2017,22 +2068,37 @@ void set_all_modules_text_ro(void) > mod->state == MODULE_STATE_GOING) > continue; > > - frob_text(&mod->core_layout, set_memory_ro); > - frob_text(&mod->init_layout, set_memory_ro); > + ret = frob_text(&mod->core_layout, set_memory_ro); > + if (ret) { > + mutex_unlock(&module_mutex); > + return ret; > + } > + ret = frob_text(&mod->init_layout, set_memory_ro); > + if (ret) { > + mutex_unlock(&module_mutex); > + return ret; > + } > } > mutex_unlock(&module_mutex); > + return 0; > } > #else /* !CONFIG_STRICT_MODULE_RWX */ > -static void module_enable_nx(const struct module *mod) { } > +static int module_enable_nx(const struct module *mod) { return 0; } > #endif /* CONFIG_STRICT_MODULE_RWX */ > -static void module_enable_x(const struct module *mod) > +static int module_enable_x(const struct module *mod) > { > - frob_text(&mod->core_layout, set_memory_x); > - frob_text(&mod->init_layout, set_memory_x); > + int ret; > + ret = frob_text(&mod->core_layout, set_memory_x); > + if (ret) > + return ret; > + ret = frob_text(&mod->init_layout, set_memory_x); > + if (ret) > + return ret; > + return 0; > } > #else /* !CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ > -static void module_enable_nx(const struct module *mod) { } > -static void module_enable_x(const struct module *mod) { } > +static int module_enable_nx(const struct module *mod) { return 0; } > +static int module_enable_x(const struct module *mod) { return 0; } > #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ > > > @@ -3534,7 +3600,11 @@ static noinline int do_init_module(struct module *mod) > /* Switch to core kallsyms now init is done: kallsyms may be walking! */ > rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); > #endif > - module_enable_ro(mod, true); > + ret = module_enable_ro(mod, true); > + if (ret) { > + mutex_unlock(&module_mutex); > + goto fail_free_freeinit; > + } > mod_tree_remove_init(mod); > module_arch_freeing_init(mod); > mod->init_layout.base = NULL; > @@ -3640,9 +3710,15 @@ static int complete_formation(struct module *mod, struct load_info *info) > /* This relies on module_mutex for list integrity. */ > module_bug_finalize(info->hdr, info->sechdrs, mod); > > - module_enable_ro(mod, false); > - module_enable_nx(mod); > - module_enable_x(mod); > + err = module_enable_ro(mod, false); > + if (err) > + goto out; > + err = module_enable_nx(mod); > + if (err) > + goto out; > + err = module_enable_x(mod); > + if (err) > + goto out; > > /* Mark state as coming so strong_try_module_get() ignores us, > * but kallsyms etc. can see us. */ > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index f9821a3374e9..d4532bb65d1b 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -5814,8 +5814,13 @@ void ftrace_module_enable(struct module *mod) > * text to read-only, as we now need to set it back to read-write > * so that we can modify the text. > */ > - if (ftrace_start_up) > - ftrace_arch_code_modify_prepare(); > + if (ftrace_start_up) { > + int ret = ftrace_arch_code_modify_prepare(); > + if (ret) { > + FTRACE_WARN_ON(ret); > + goto out_unlock; > + } Can FTRACE_WARN_ON() be used in an "if" like WARN_ON? This could maybe look like: ret = ftrace_arch... if (FTRACE_WARN_ON(ret)) goto out_unlock Though I not this doesn't result in an error getting passed up? i.e. ftrace_module_enable() is still "void". Does that matter here? > + } > > do_for_each_ftrace_rec(pg, rec) { > int cnt; > @@ -5854,8 +5859,10 @@ void ftrace_module_enable(struct module *mod) > } while_for_each_ftrace_rec(); > > out_loop: > - if (ftrace_start_up) > - ftrace_arch_code_modify_post_process(); > + if (ftrace_start_up) { > + int ret = ftrace_arch_code_modify_post_process(); > + FTRACE_WARN_ON(ret); > + } > > out_unlock: > mutex_unlock(&ftrace_lock); > -- > 2.17.1 > Thanks!
On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: > Right now several architectures allow their set_memory_*() family of > functions to fail, but callers may not be checking the return values. We > need to fix the callers and add the __must_check attribute. They also may > not provide any level of atomicity, in the sense that the memory > protections may be left incomplete on failure. This issue likely has a few > steps on effects architectures[1]: > 1)Have all callers of set_memory_*() helpers check the return value. > 2)Add __much_check to all set_memory_*() helpers so that new uses do not > ignore the return value. > 3)Add atomicity to the calls so that the memory protections aren't left in > a partial state. > > Ideally, the failure of set_memory_*() should be passed up the call stack, > and callers should examine the failure and deal with it. But currently, > some callers just have void return type. > > We need to fix the callers to handle the return all the way to the top of > stack, and it will require a large series of patches to finish all the three > steps mentioned above. I start with kernel/module, and will move onto other > subsystems. I am not entirely sure about the failure modes for each caller. > So I would like to get some comments before I move forward. This single > patch is just for fixing the return value of set_memory_*() function in > kernel/module, and also the related callers. Any feedback would be greatly > appreciated. Please have a look here: https://lkml.kernel.org/r/20191111131252.921588318@infradead.org Much of the code you're patching is slated for removal. Josh also has patches reworking KLP and there's some ARM64 patches pending at which point we can also delete module_disable_ro().
On Tue, Nov 19, 2019 at 09:07:34AM -0800, Kees Cook wrote: > > diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c > > index bda949fd84e8..7ea1338821d6 100644 > > --- a/arch/arm/kernel/ftrace.c > > +++ b/arch/arm/kernel/ftrace.c > > @@ -59,13 +59,15 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr) > > > > int ftrace_arch_code_modify_prepare(void) > > { > > - set_all_modules_text_rw(); > > - return 0; > > + return set_all_modules_text_rw(); > > } > > > > int ftrace_arch_code_modify_post_process(void) > > { > > - set_all_modules_text_ro(); > > + int ret; > > Blank line here... > > > + ret = set_all_modules_text_ro(); > > + if (ret) > > + return ret; > > /* Make sure any TLB misses during machine stop are cleared. */ > > flush_tlb_all(); > > return 0; > > Are callers of these ftrace functions checking return values too? Aside from the fact that I just deleted the set_all_modules_text_*() functions, ftrace actually does check the return value. It prvides a nice WARN and kills ftrace dead. > > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c > > index 171773257974..97a89c38f6b9 100644 > > --- a/arch/arm64/kernel/ftrace.c > > +++ b/arch/arm64/kernel/ftrace.c > > @@ -115,9 +115,11 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > > } > > > > /* point the trampoline to our ftrace entry point */ > > - module_disable_ro(mod); > > + if (module_disable_ro(mod)) > > + return -EINVAL; > > *dst = trampoline; > > - module_enable_ro(mod, true); > > + if (module_enable_ro(mod, true)) > > + return -EINVAL; > > > > /* > > * Ensure updated trampoline is visible to instruction Patching dead code again: https://lkml.kernel.org/r/20191029165832.33606-5-mark.rutland@arm.com
> On Nov 19, 2019, at 11:07 AM, Kees Cook <keescook@chromium.org> wrote: > > On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: >> Right now several architectures allow their set_memory_*() family of >> functions to fail, but callers may not be checking the return values. We >> need to fix the callers and add the __must_check attribute. They also may >> not provide any level of atomicity, in the sense that the memory >> protections may be left incomplete on failure. This issue likely has a few >> steps on effects architectures[1]: > > Awesome; thanks for working on this! > > A few suggestions on this patch, which will help reviewers, below... > >> 1)Have all callers of set_memory_*() helpers check the return value. >> 2)Add __much_check to all set_memory_*() helpers so that new uses do not >> ignore the return value. >> 3)Add atomicity to the calls so that the memory protections aren't left in >> a partial state. > > Maybe clarify to say something like "this series is step 1”? ok. Will add the clarification. Thanks! > >> >> Ideally, the failure of set_memory_*() should be passed up the call stack, >> and callers should examine the failure and deal with it. But currently, >> some callers just have void return type. >> >> We need to fix the callers to handle the return all the way to the top of >> stack, and it will require a large series of patches to finish all the three >> steps mentioned above. I start with kernel/module, and will move onto other >> subsystems. I am not entirely sure about the failure modes for each caller. >> So I would like to get some comments before I move forward. This single >> patch is just for fixing the return value of set_memory_*() function in >> kernel/module, and also the related callers. Any feedback would be greatly >> appreciated. >> >> [1]:https://github.com/KSPP/linux/issues/7 >> >> Signed-off-by: Tianlin Li <tli@digitalocean.com> >> --- >> arch/arm/kernel/ftrace.c | 8 +- >> arch/arm64/kernel/ftrace.c | 6 +- >> arch/nds32/kernel/ftrace.c | 6 +- >> arch/x86/kernel/ftrace.c | 13 ++- >> include/linux/module.h | 16 ++-- >> kernel/livepatch/core.c | 15 +++- >> kernel/module.c | 170 +++++++++++++++++++++++++++---------- >> kernel/trace/ftrace.c | 15 +++- >> 8 files changed, 175 insertions(+), 74 deletions(-) > > - Can you break this patch into 3 separate patches, by "subsystem": > - ftrace > - module > - livepatch (unless Jessica thinks maybe livepatch should go > with module?) > - Please run patches through scripts/checkpatch.pl to catch common > coding style issues (e.g. blank line between variable declarations > and function body). > - These changes look pretty straight forward, so I think it'd be fine > to drop the "RFC" tag and include linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org> in the > Cc list for the v2. For lots of detail, see: > https://www.kernel.org/doc/html/latest/process/submitting-patches.html <https://www.kernel.org/doc/html/latest/process/submitting-patches.html> ok. I will fix them in v2. Thanks a lot! > More below... > >> >> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c >> index bda949fd84e8..7ea1338821d6 100644 >> --- a/arch/arm/kernel/ftrace.c >> +++ b/arch/arm/kernel/ftrace.c >> @@ -59,13 +59,15 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr) >> >> int ftrace_arch_code_modify_prepare(void) >> { >> - set_all_modules_text_rw(); >> - return 0; >> + return set_all_modules_text_rw(); >> } >> >> int ftrace_arch_code_modify_post_process(void) >> { >> - set_all_modules_text_ro(); >> + int ret; > > Blank line here... > >> + ret = set_all_modules_text_ro(); >> + if (ret) >> + return ret; >> /* Make sure any TLB misses during machine stop are cleared. */ >> flush_tlb_all(); >> return 0; > > Are callers of these ftrace functions checking return values too? ftrace_arch_code_modify_prepare is called in ftrace_run_update_code and ftrace_module_enable. ftrace_run_update_code is checking the return value of ftrace_arch_code_modify_prepare already. ftrace_module_enable didn’t check. (I modifies this function in this patch to check the return value of ftrace_arch_code_modify_prepare ). Same for ftrace_arch_code_modify_post_process. >> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c >> index 171773257974..97a89c38f6b9 100644 >> --- a/arch/arm64/kernel/ftrace.c >> +++ b/arch/arm64/kernel/ftrace.c >> @@ -115,9 +115,11 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) >> } >> >> /* point the trampoline to our ftrace entry point */ >> - module_disable_ro(mod); >> + if (module_disable_ro(mod)) >> + return -EINVAL; >> *dst = trampoline; >> - module_enable_ro(mod, true); >> + if (module_enable_ro(mod, true)) >> + return -EINVAL; >> >> /* >> * Ensure updated trampoline is visible to instruction >> diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c >> index fd2a54b8cd57..e9e63e703a3e 100644 >> --- a/arch/nds32/kernel/ftrace.c >> +++ b/arch/nds32/kernel/ftrace.c >> @@ -91,14 +91,12 @@ int __init ftrace_dyn_arch_init(void) >> >> int ftrace_arch_code_modify_prepare(void) >> { >> - set_all_modules_text_rw(); >> - return 0; >> + return set_all_modules_text_rw(); >> } >> >> int ftrace_arch_code_modify_post_process(void) >> { >> - set_all_modules_text_ro(); >> - return 0; >> + return set_all_modules_text_ro(); >> } >> >> static unsigned long gen_sethi_insn(unsigned long addr) >> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c >> index 024c3053dbba..7fdee06e2a76 100644 >> --- a/arch/x86/kernel/ftrace.c >> +++ b/arch/x86/kernel/ftrace.c >> @@ -42,19 +42,26 @@ int ftrace_arch_code_modify_prepare(void) >> * and live kernel patching from changing the text permissions while >> * ftrace has it set to "read/write". >> */ >> + int ret; >> mutex_lock(&text_mutex); >> set_kernel_text_rw(); >> - set_all_modules_text_rw(); >> + ret = set_all_modules_text_rw(); >> + if (ret) { >> + set_kernel_text_ro(); > > Is the set_kernel_text_ro() here is an atomicity fix? Also, won't a future > patch need to check the result of that function call? (i.e. should it > be left out of this series and should atomicity fixes happen inside the > set_memory_*() functions? Can they happen there?) set_kernel_text_tro() here was not for an atomicity fix in step 3, it was just for reverting the status in ftrace_arch_code_modify_prepare, because set_kernel_text_rw() is called. Thanks for pointing it out. I will check it. > >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> return 0; >> } >> >> int ftrace_arch_code_modify_post_process(void) >> __releases(&text_mutex) >> { >> - set_all_modules_text_ro(); >> + int ret; > > blank line needed... > >> + ret = set_all_modules_text_ro(); >> set_kernel_text_ro(); >> mutex_unlock(&text_mutex); >> - return 0; >> + return ret; >> } >> >> union ftrace_code_union { >> diff --git a/include/linux/module.h b/include/linux/module.h >> index 1455812dd325..e6c7f3b719a3 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -847,15 +847,15 @@ extern int module_sysfs_initialized; >> #define __MODULE_STRING(x) __stringify(x) >> >> #ifdef CONFIG_STRICT_MODULE_RWX >> -extern void set_all_modules_text_rw(void); >> -extern void set_all_modules_text_ro(void); >> -extern void module_enable_ro(const struct module *mod, bool after_init); >> -extern void module_disable_ro(const struct module *mod); >> +extern int set_all_modules_text_rw(void); >> +extern int set_all_modules_text_ro(void); >> +extern int module_enable_ro(const struct module *mod, bool after_init); >> +extern int module_disable_ro(const struct module *mod); >> #else >> -static inline void set_all_modules_text_rw(void) { } >> -static inline void set_all_modules_text_ro(void) { } >> -static inline void module_enable_ro(const struct module *mod, bool after_init) { } >> -static inline void module_disable_ro(const struct module *mod) { } >> +static inline int set_all_modules_text_rw(void) { return 0; } >> +static inline int set_all_modules_text_ro(void) { return 0; } >> +static inline int module_enable_ro(const struct module *mod, bool after_init) { return 0; } > > Please wrap this line (yes, the old one wasn't...) > >> +static inline int module_disable_ro(const struct module *mod) { return 0; } >> #endif >> >> #ifdef CONFIG_GENERIC_BUG >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >> index c4ce08f43bd6..39bfc0685854 100644 >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct klp_patch *patch, >> >> mutex_lock(&text_mutex); >> >> - module_disable_ro(patch->mod); >> + ret = module_disable_ro(patch->mod); >> + if (ret) { >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> ret = klp_write_object_relocations(patch->mod, obj); >> if (ret) { >> - module_enable_ro(patch->mod, true); >> + if (module_enable_ro(patch->mod, true)); >> + pr_err("module_enable_ro failed.\n"); > > Why the pr_err() here and not in other failure cases? (Also, should it > instead be a WARN_ONCE() inside the set_memory_*() functions instead?) Because module_enable_ro() is called in the checking of another return value. I can not return the value, so I print the error. Similar to the previous question, should it be fixed by atomicity patches later? > >> mutex_unlock(&text_mutex); >> return ret; >> } >> >> arch_klp_init_object_loaded(patch, obj); >> - module_enable_ro(patch->mod, true); >> + ret = module_enable_ro(patch->mod, true); >> + if (ret) { >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> >> mutex_unlock(&text_mutex); >> >> diff --git a/kernel/module.c b/kernel/module.c >> index 9ee93421269c..87125b5e315c 100644 >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -1900,111 +1900,162 @@ static void mod_sysfs_teardown(struct module *mod) >> * >> * These values are always page-aligned (as is base) >> */ >> -static void frob_text(const struct module_layout *layout, >> +static int frob_text(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, int num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base, >> + return set_memory((unsigned long)layout->base, >> layout->text_size >> PAGE_SHIFT); >> } >> >> #ifdef CONFIG_STRICT_MODULE_RWX >> -static void frob_rodata(const struct module_layout *layout, >> +static int frob_rodata(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, int num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base + layout->text_size, >> + return set_memory((unsigned long)layout->base + layout->text_size, >> (layout->ro_size - layout->text_size) >> PAGE_SHIFT); >> } >> >> -static void frob_ro_after_init(const struct module_layout *layout, >> +static int frob_ro_after_init(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, int num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base + layout->ro_size, >> + return set_memory((unsigned long)layout->base + layout->ro_size, >> (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT); >> } >> >> -static void frob_writable_data(const struct module_layout *layout, >> +static int frob_writable_data(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, int num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base + layout->ro_after_init_size, >> + return set_memory((unsigned long)layout->base + layout->ro_after_init_size, >> (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT); >> } >> >> /* livepatching wants to disable read-only so it can frob module. */ >> -void module_disable_ro(const struct module *mod) >> +int module_disable_ro(const struct module *mod) >> { >> + int ret; >> if (!rodata_enabled) >> - return; >> + return 0; >> >> - frob_text(&mod->core_layout, set_memory_rw); >> - frob_rodata(&mod->core_layout, set_memory_rw); >> - frob_ro_after_init(&mod->core_layout, set_memory_rw); >> - frob_text(&mod->init_layout, set_memory_rw); >> - frob_rodata(&mod->init_layout, set_memory_rw); >> + ret = frob_text(&mod->core_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret = frob_rodata(&mod->core_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret = frob_ro_after_init(&mod->core_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret = frob_text(&mod->init_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret = frob_rodata(&mod->init_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + >> + return 0; >> } >> >> -void module_enable_ro(const struct module *mod, bool after_init) >> +int module_enable_ro(const struct module *mod, bool after_init) >> { >> + int ret; >> if (!rodata_enabled) >> - return; >> + return 0; >> >> set_vm_flush_reset_perms(mod->core_layout.base); >> set_vm_flush_reset_perms(mod->init_layout.base); >> - frob_text(&mod->core_layout, set_memory_ro); >> + ret = frob_text(&mod->core_layout, set_memory_ro); >> + if (ret) >> + return ret; >> >> - frob_rodata(&mod->core_layout, set_memory_ro); >> - frob_text(&mod->init_layout, set_memory_ro); >> - frob_rodata(&mod->init_layout, set_memory_ro); >> + ret = frob_rodata(&mod->core_layout, set_memory_ro); >> + if (ret) >> + return ret; >> + ret = frob_text(&mod->init_layout, set_memory_ro); >> + if (ret) >> + return ret; >> + ret = frob_rodata(&mod->init_layout, set_memory_ro); >> + if (ret) >> + return ret; >> >> - if (after_init) >> - frob_ro_after_init(&mod->core_layout, set_memory_ro); >> + if (after_init) { >> + ret = frob_ro_after_init(&mod->core_layout, set_memory_ro); >> + if (ret) >> + return ret; >> + } >> + return 0; >> } >> >> -static void module_enable_nx(const struct module *mod) >> +static int module_enable_nx(const struct module *mod) >> { >> - frob_rodata(&mod->core_layout, set_memory_nx); >> - frob_ro_after_init(&mod->core_layout, set_memory_nx); >> - frob_writable_data(&mod->core_layout, set_memory_nx); >> - frob_rodata(&mod->init_layout, set_memory_nx); >> - frob_writable_data(&mod->init_layout, set_memory_nx); >> + int ret; >> + >> + ret = frob_rodata(&mod->core_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret = frob_ro_after_init(&mod->core_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret = frob_writable_data(&mod->core_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret = frob_rodata(&mod->init_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret = frob_writable_data(&mod->init_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + >> + return 0; >> } >> >> /* Iterate through all modules and set each module's text as RW */ >> -void set_all_modules_text_rw(void) >> +int set_all_modules_text_rw(void) >> { >> struct module *mod; >> + int ret; >> >> if (!rodata_enabled) >> - return; >> + return 0; >> >> mutex_lock(&module_mutex); >> list_for_each_entry_rcu(mod, &modules, list) { >> if (mod->state == MODULE_STATE_UNFORMED) >> continue; >> >> - frob_text(&mod->core_layout, set_memory_rw); >> - frob_text(&mod->init_layout, set_memory_rw); >> + ret = frob_text(&mod->core_layout, set_memory_rw); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >> + ret = frob_text(&mod->init_layout, set_memory_rw); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } > > This pattern feels like it might be better with a "goto out" style: > > mutex_lock... > list_for_each... { > ret = frob_... > if (ret) > goto out; > ret = frob_... > if (ret) > goto out; > } > ret = 0; > out: > mutex_unlock... > return ret; Thanks! Will fix it in v2. > >> } >> mutex_unlock(&module_mutex); >> + return 0; >> } >> >> /* Iterate through all modules and set each module's text as RO */ >> -void set_all_modules_text_ro(void) >> +int set_all_modules_text_ro(void) >> { >> struct module *mod; >> + int ret; >> >> if (!rodata_enabled) >> - return; >> + return 0; >> >> mutex_lock(&module_mutex); >> list_for_each_entry_rcu(mod, &modules, list) { >> @@ -2017,22 +2068,37 @@ void set_all_modules_text_ro(void) >> mod->state == MODULE_STATE_GOING) >> continue; >> >> - frob_text(&mod->core_layout, set_memory_ro); >> - frob_text(&mod->init_layout, set_memory_ro); >> + ret = frob_text(&mod->core_layout, set_memory_ro); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >> + ret = frob_text(&mod->init_layout, set_memory_ro); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >> } >> mutex_unlock(&module_mutex); >> + return 0; >> } >> #else /* !CONFIG_STRICT_MODULE_RWX */ >> -static void module_enable_nx(const struct module *mod) { } >> +static int module_enable_nx(const struct module *mod) { return 0; } >> #endif /* CONFIG_STRICT_MODULE_RWX */ >> -static void module_enable_x(const struct module *mod) >> +static int module_enable_x(const struct module *mod) >> { >> - frob_text(&mod->core_layout, set_memory_x); >> - frob_text(&mod->init_layout, set_memory_x); >> + int ret; >> + ret = frob_text(&mod->core_layout, set_memory_x); >> + if (ret) >> + return ret; >> + ret = frob_text(&mod->init_layout, set_memory_x); >> + if (ret) >> + return ret; >> + return 0; >> } >> #else /* !CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ >> -static void module_enable_nx(const struct module *mod) { } >> -static void module_enable_x(const struct module *mod) { } >> +static int module_enable_nx(const struct module *mod) { return 0; } >> +static int module_enable_x(const struct module *mod) { return 0; } >> #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ >> >> >> @@ -3534,7 +3600,11 @@ static noinline int do_init_module(struct module *mod) >> /* Switch to core kallsyms now init is done: kallsyms may be walking! */ >> rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); >> #endif >> - module_enable_ro(mod, true); >> + ret = module_enable_ro(mod, true); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + goto fail_free_freeinit; >> + } >> mod_tree_remove_init(mod); >> module_arch_freeing_init(mod); >> mod->init_layout.base = NULL; >> @@ -3640,9 +3710,15 @@ static int complete_formation(struct module *mod, struct load_info *info) >> /* This relies on module_mutex for list integrity. */ >> module_bug_finalize(info->hdr, info->sechdrs, mod); >> >> - module_enable_ro(mod, false); >> - module_enable_nx(mod); >> - module_enable_x(mod); >> + err = module_enable_ro(mod, false); >> + if (err) >> + goto out; >> + err = module_enable_nx(mod); >> + if (err) >> + goto out; >> + err = module_enable_x(mod); >> + if (err) >> + goto out; >> >> /* Mark state as coming so strong_try_module_get() ignores us, >> * but kallsyms etc. can see us. */ >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> index f9821a3374e9..d4532bb65d1b 100644 >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -5814,8 +5814,13 @@ void ftrace_module_enable(struct module *mod) >> * text to read-only, as we now need to set it back to read-write >> * so that we can modify the text. >> */ >> - if (ftrace_start_up) >> - ftrace_arch_code_modify_prepare(); >> + if (ftrace_start_up) { >> + int ret = ftrace_arch_code_modify_prepare(); >> + if (ret) { >> + FTRACE_WARN_ON(ret); >> + goto out_unlock; >> + } > > Can FTRACE_WARN_ON() be used in an "if" like WARN_ON? This could maybe > look like: > > ret = ftrace_arch... > if (FTRACE_WARN_ON(ret)) > goto out_unlock > > Though I not this doesn't result in an error getting passed up? i.e. > ftrace_module_enable() is still "void". Does that matter here? Thanks. I will fix it in v2. >> + } >> >> do_for_each_ftrace_rec(pg, rec) { >> int cnt; >> @@ -5854,8 +5859,10 @@ void ftrace_module_enable(struct module *mod) >> } while_for_each_ftrace_rec(); >> >> out_loop: >> - if (ftrace_start_up) >> - ftrace_arch_code_modify_post_process(); >> + if (ftrace_start_up) { >> + int ret = ftrace_arch_code_modify_post_process(); >> + FTRACE_WARN_ON(ret); >> + } >> >> out_unlock: >> mutex_unlock(&ftrace_lock); >> -- >> 2.17.1 >> > > Thanks, Thanks for all detail suggestions! > -- > Kees Cook
> On Nov 19, 2019, at 3:38 PM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: >> Right now several architectures allow their set_memory_*() family of >> functions to fail, but callers may not be checking the return values. We >> need to fix the callers and add the __must_check attribute. They also may >> not provide any level of atomicity, in the sense that the memory >> protections may be left incomplete on failure. This issue likely has a few >> steps on effects architectures[1]: >> 1)Have all callers of set_memory_*() helpers check the return value. >> 2)Add __much_check to all set_memory_*() helpers so that new uses do not >> ignore the return value. >> 3)Add atomicity to the calls so that the memory protections aren't left in >> a partial state. >> >> Ideally, the failure of set_memory_*() should be passed up the call stack, >> and callers should examine the failure and deal with it. But currently, >> some callers just have void return type. >> >> We need to fix the callers to handle the return all the way to the top of >> stack, and it will require a large series of patches to finish all the three >> steps mentioned above. I start with kernel/module, and will move onto other >> subsystems. I am not entirely sure about the failure modes for each caller. >> So I would like to get some comments before I move forward. This single >> patch is just for fixing the return value of set_memory_*() function in >> kernel/module, and also the related callers. Any feedback would be greatly >> appreciated. > > Please have a look here: > > https://lkml.kernel.org/r/20191111131252.921588318@infradead.org <https://lkml.kernel.org/r/20191111131252.921588318@infradead.org> > > Much of the code you're patching is slated for removal. > > Josh also has patches reworking KLP and there's some ARM64 patches > pending at which point we can also delete module_disable_ro(). Thanks for the information. I will check the code.
On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: > Right now several architectures allow their set_memory_*() family of > functions to fail, but callers may not be checking the return values. We > need to fix the callers and add the __must_check attribute. Please formulate commit messages in passive tone. "we" is ambiguous. From Documentation/process/submitting-patches.rst: "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour." Also, you could add a high-level summary of the failure case from: https://lore.kernel.org/netdev/20180628213459.28631-4-daniel@iogearbox.net/ as a more real-life, convincing justification for this. > They also may not provide any level of atomicity, in the sense that > the memory protections may be left incomplete on failure. > This issue likely has a few > steps on effects architectures[1]: > 1)Have all callers of set_memory_*() helpers check the return value. > 2)Add __much_check to all set_memory_*() helpers so that new uses do not __must_check > ignore the return value. > 3)Add atomicity to the calls so that the memory protections aren't left in > a partial state. > > Ideally, the failure of set_memory_*() should be passed up the call stack, > and callers should examine the failure and deal with it. But currently, > some callers just have void return type. > > We need to fix the callers to handle the return all the way to the top of > stack, and it will require a large series of patches to finish all the three > steps mentioned above. I start with kernel/module, and will move onto other > subsystems. I am not entirely sure about the failure modes for each caller. > So I would like to get some comments before I move forward. This single > patch is just for fixing the return value of set_memory_*() function in > kernel/module, and also the related callers. Any feedback would be greatly > appreciated. > > [1]:https://github.com/KSPP/linux/issues/7 > > Signed-off-by: Tianlin Li <tli@digitalocean.com> > --- > arch/arm/kernel/ftrace.c | 8 +- > arch/arm64/kernel/ftrace.c | 6 +- > arch/nds32/kernel/ftrace.c | 6 +- > arch/x86/kernel/ftrace.c | 13 ++- > include/linux/module.h | 16 ++-- > kernel/livepatch/core.c | 15 +++- > kernel/module.c | 170 +++++++++++++++++++++++++++---------- > kernel/trace/ftrace.c | 15 +++- > 8 files changed, 175 insertions(+), 74 deletions(-) Yeah, general idea makes sense but you'd need to redo your patch ontop of linux-next because there are some changes in flight in ftrace-land at least and your patch won't apply anymore after next week, when the merge window opens. Also, you should use checkpatch before sending a patch as sometimes it makes sense what it complains about: WARNING: Missing a blank line after declarations #79: FILE: arch/arm/kernel/ftrace.c:68: + int ret; + ret = set_all_modules_text_ro(); WARNING: Missing a blank line after declarations #150: FILE: arch/x86/kernel/ftrace.c:61: + int ret; + ret = set_all_modules_text_ro(); WARNING: trailing semicolon indicates no statements, indent implies otherwise #203: FILE: kernel/livepatch/core.c:731: + if (module_enable_ro(patch->mod, true)); + pr_err("module_enable_ro failed.\n"); ERROR: trailing statements should be on next line #203: FILE: kernel/livepatch/core.c:731: + if (module_enable_ro(patch->mod, true)); WARNING: Missing a blank line after declarations #451: FILE: kernel/module.c:2091: + int ret; + ret = frob_text(&mod->core_layout, set_memory_x); WARNING: Missing a blank line after declarations #511: FILE: kernel/trace/ftrace.c:5819: + int ret = ftrace_arch_code_modify_prepare(); + if (ret) { WARNING: Missing a blank line after declarations #527: FILE: kernel/trace/ftrace.c:5864: + int ret = ftrace_arch_code_modify_post_process(); + FTRACE_WARN_ON(ret); > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index c4ce08f43bd6..39bfc0685854 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct klp_patch *patch, > > mutex_lock(&text_mutex); > > - module_disable_ro(patch->mod); > + ret = module_disable_ro(patch->mod); > + if (ret) { > + mutex_unlock(&text_mutex); > + return ret; > + } > ret = klp_write_object_relocations(patch->mod, obj); > if (ret) { > - module_enable_ro(patch->mod, true); > + if (module_enable_ro(patch->mod, true)); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ and if you look at its output above closely, it might even help you catch the bug you've added. [ Don't worry, happens to the best of us. :-) ] Also, what would help review is if you split your patch: patch 1: Change functions to return a retval patch 2-n: Change call sites to handle retval properly Thx.
> On Nov 20, 2019, at 3:58 AM, Borislav Petkov <bp@alien8.de> wrote: > > On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: >> Right now several architectures allow their set_memory_*() family of >> functions to fail, but callers may not be checking the return values. We >> need to fix the callers and add the __must_check attribute. > > Please formulate commit messages in passive tone. "we" is ambiguous. > > From Documentation/process/submitting-patches.rst: > > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour." > > Also, you could add a high-level summary of the failure case from: > > https://lore.kernel.org/netdev/20180628213459.28631-4-daniel@iogearbox.net/ <https://lore.kernel.org/netdev/20180628213459.28631-4-daniel@iogearbox.net/> > > as a more real-life, convincing justification for this. Thanks for pointing it out. I will fix them in v2. >> They also may not provide any level of atomicity, in the sense that >> the memory protections may be left incomplete on failure. >> This issue likely has a few >> steps on effects architectures[1]: >> 1)Have all callers of set_memory_*() helpers check the return value. >> 2)Add __much_check to all set_memory_*() helpers so that new uses do not > > __must_check > >> ignore the return value. >> 3)Add atomicity to the calls so that the memory protections aren't left in >> a partial state. >> >> Ideally, the failure of set_memory_*() should be passed up the call stack, >> and callers should examine the failure and deal with it. But currently, >> some callers just have void return type. >> >> We need to fix the callers to handle the return all the way to the top of >> stack, and it will require a large series of patches to finish all the three >> steps mentioned above. I start with kernel/module, and will move onto other >> subsystems. I am not entirely sure about the failure modes for each caller. >> So I would like to get some comments before I move forward. This single >> patch is just for fixing the return value of set_memory_*() function in >> kernel/module, and also the related callers. Any feedback would be greatly >> appreciated. >> >> [1]:https://github.com/KSPP/linux/issues/7 >> >> Signed-off-by: Tianlin Li <tli@digitalocean.com> >> --- >> arch/arm/kernel/ftrace.c | 8 +- >> arch/arm64/kernel/ftrace.c | 6 +- >> arch/nds32/kernel/ftrace.c | 6 +- >> arch/x86/kernel/ftrace.c | 13 ++- >> include/linux/module.h | 16 ++-- >> kernel/livepatch/core.c | 15 +++- >> kernel/module.c | 170 +++++++++++++++++++++++++++---------- >> kernel/trace/ftrace.c | 15 +++- >> 8 files changed, 175 insertions(+), 74 deletions(-) > > Yeah, general idea makes sense but you'd need to redo your patch ontop > of linux-next because there are some changes in flight in ftrace-land at > least and your patch won't apply anymore after next week, when the merge > window opens. > > Also, you should use checkpatch before sending a patch as sometimes it makes > sense what it complains about: > > WARNING: Missing a blank line after declarations > #79: FILE: arch/arm/kernel/ftrace.c:68: > + int ret; > + ret = set_all_modules_text_ro(); > > WARNING: Missing a blank line after declarations > #150: FILE: arch/x86/kernel/ftrace.c:61: > + int ret; > + ret = set_all_modules_text_ro(); > > WARNING: trailing semicolon indicates no statements, indent implies otherwise > #203: FILE: kernel/livepatch/core.c:731: > + if (module_enable_ro(patch->mod, true)); > + pr_err("module_enable_ro failed.\n"); > > ERROR: trailing statements should be on next line > #203: FILE: kernel/livepatch/core.c:731: > + if (module_enable_ro(patch->mod, true)); > > WARNING: Missing a blank line after declarations > #451: FILE: kernel/module.c:2091: > + int ret; > + ret = frob_text(&mod->core_layout, set_memory_x); > > WARNING: Missing a blank line after declarations > #511: FILE: kernel/trace/ftrace.c:5819: > + int ret = ftrace_arch_code_modify_prepare(); > + if (ret) { > > WARNING: Missing a blank line after declarations > #527: FILE: kernel/trace/ftrace.c:5864: > + int ret = ftrace_arch_code_modify_post_process(); > + FTRACE_WARN_ON(ret); > >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >> index c4ce08f43bd6..39bfc0685854 100644 >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct klp_patch *patch, >> >> mutex_lock(&text_mutex); >> >> - module_disable_ro(patch->mod); >> + ret = module_disable_ro(patch->mod); >> + if (ret) { >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> ret = klp_write_object_relocations(patch->mod, obj); >> if (ret) { >> - module_enable_ro(patch->mod, true); >> + if (module_enable_ro(patch->mod, true)); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > and if you look at its output above closely, it might even help you > catch the bug you've added. > > [ Don't worry, happens to the best of us. :-) ] > > Also, what would help review is if you split your patch: > > patch 1: Change functions to return a retval > patch 2-n: Change call sites to handle retval properly ok. I will redo the patch on top of linux-next. I will use checkpatch and split the patch properly in v2. Thanks for all valuable comments. Really appreciate it. > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette <https://people.kernel.org/tglx/notes-about-netiquette>
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c index bda949fd84e8..7ea1338821d6 100644 --- a/arch/arm/kernel/ftrace.c +++ b/arch/arm/kernel/ftrace.c @@ -59,13 +59,15 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr) int ftrace_arch_code_modify_prepare(void) { - set_all_modules_text_rw(); - return 0; + return set_all_modules_text_rw(); } int ftrace_arch_code_modify_post_process(void) { - set_all_modules_text_ro(); + int ret; + ret = set_all_modules_text_ro(); + if (ret) + return ret; /* Make sure any TLB misses during machine stop are cleared. */ flush_tlb_all(); return 0; diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c index 171773257974..97a89c38f6b9 100644 --- a/arch/arm64/kernel/ftrace.c +++ b/arch/arm64/kernel/ftrace.c @@ -115,9 +115,11 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) } /* point the trampoline to our ftrace entry point */ - module_disable_ro(mod); + if (module_disable_ro(mod)) + return -EINVAL; *dst = trampoline; - module_enable_ro(mod, true); + if (module_enable_ro(mod, true)) + return -EINVAL; /* * Ensure updated trampoline is visible to instruction diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c index fd2a54b8cd57..e9e63e703a3e 100644 --- a/arch/nds32/kernel/ftrace.c +++ b/arch/nds32/kernel/ftrace.c @@ -91,14 +91,12 @@ int __init ftrace_dyn_arch_init(void) int ftrace_arch_code_modify_prepare(void) { - set_all_modules_text_rw(); - return 0; + return set_all_modules_text_rw(); } int ftrace_arch_code_modify_post_process(void) { - set_all_modules_text_ro(); - return 0; + return set_all_modules_text_ro(); } static unsigned long gen_sethi_insn(unsigned long addr) diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 024c3053dbba..7fdee06e2a76 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -42,19 +42,26 @@ int ftrace_arch_code_modify_prepare(void) * and live kernel patching from changing the text permissions while * ftrace has it set to "read/write". */ + int ret; mutex_lock(&text_mutex); set_kernel_text_rw(); - set_all_modules_text_rw(); + ret = set_all_modules_text_rw(); + if (ret) { + set_kernel_text_ro(); + mutex_unlock(&text_mutex); + return ret; + } return 0; } int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex) { - set_all_modules_text_ro(); + int ret; + ret = set_all_modules_text_ro(); set_kernel_text_ro(); mutex_unlock(&text_mutex); - return 0; + return ret; } union ftrace_code_union { diff --git a/include/linux/module.h b/include/linux/module.h index 1455812dd325..e6c7f3b719a3 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -847,15 +847,15 @@ extern int module_sysfs_initialized; #define __MODULE_STRING(x) __stringify(x) #ifdef CONFIG_STRICT_MODULE_RWX -extern void set_all_modules_text_rw(void); -extern void set_all_modules_text_ro(void); -extern void module_enable_ro(const struct module *mod, bool after_init); -extern void module_disable_ro(const struct module *mod); +extern int set_all_modules_text_rw(void); +extern int set_all_modules_text_ro(void); +extern int module_enable_ro(const struct module *mod, bool after_init); +extern int module_disable_ro(const struct module *mod); #else -static inline void set_all_modules_text_rw(void) { } -static inline void set_all_modules_text_ro(void) { } -static inline void module_enable_ro(const struct module *mod, bool after_init) { } -static inline void module_disable_ro(const struct module *mod) { } +static inline int set_all_modules_text_rw(void) { return 0; } +static inline int set_all_modules_text_ro(void) { return 0; } +static inline int module_enable_ro(const struct module *mod, bool after_init) { return 0; } +static inline int module_disable_ro(const struct module *mod) { return 0; } #endif #ifdef CONFIG_GENERIC_BUG diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index c4ce08f43bd6..39bfc0685854 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct klp_patch *patch, mutex_lock(&text_mutex); - module_disable_ro(patch->mod); + ret = module_disable_ro(patch->mod); + if (ret) { + mutex_unlock(&text_mutex); + return ret; + } ret = klp_write_object_relocations(patch->mod, obj); if (ret) { - module_enable_ro(patch->mod, true); + if (module_enable_ro(patch->mod, true)); + pr_err("module_enable_ro failed.\n"); mutex_unlock(&text_mutex); return ret; } arch_klp_init_object_loaded(patch, obj); - module_enable_ro(patch->mod, true); + ret = module_enable_ro(patch->mod, true); + if (ret) { + mutex_unlock(&text_mutex); + return ret; + } mutex_unlock(&text_mutex); diff --git a/kernel/module.c b/kernel/module.c index 9ee93421269c..87125b5e315c 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1900,111 +1900,162 @@ static void mod_sysfs_teardown(struct module *mod) * * These values are always page-aligned (as is base) */ -static void frob_text(const struct module_layout *layout, +static int frob_text(const struct module_layout *layout, int (*set_memory)(unsigned long start, int num_pages)) { BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); - set_memory((unsigned long)layout->base, + return set_memory((unsigned long)layout->base, layout->text_size >> PAGE_SHIFT); } #ifdef CONFIG_STRICT_MODULE_RWX -static void frob_rodata(const struct module_layout *layout, +static int frob_rodata(const struct module_layout *layout, int (*set_memory)(unsigned long start, int num_pages)) { BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); - set_memory((unsigned long)layout->base + layout->text_size, + return set_memory((unsigned long)layout->base + layout->text_size, (layout->ro_size - layout->text_size) >> PAGE_SHIFT); } -static void frob_ro_after_init(const struct module_layout *layout, +static int frob_ro_after_init(const struct module_layout *layout, int (*set_memory)(unsigned long start, int num_pages)) { BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1)); - set_memory((unsigned long)layout->base + layout->ro_size, + return set_memory((unsigned long)layout->base + layout->ro_size, (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT); } -static void frob_writable_data(const struct module_layout *layout, +static int frob_writable_data(const struct module_layout *layout, int (*set_memory)(unsigned long start, int num_pages)) { BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1)); BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1)); - set_memory((unsigned long)layout->base + layout->ro_after_init_size, + return set_memory((unsigned long)layout->base + layout->ro_after_init_size, (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT); } /* livepatching wants to disable read-only so it can frob module. */ -void module_disable_ro(const struct module *mod) +int module_disable_ro(const struct module *mod) { + int ret; if (!rodata_enabled) - return; + return 0; - frob_text(&mod->core_layout, set_memory_rw); - frob_rodata(&mod->core_layout, set_memory_rw); - frob_ro_after_init(&mod->core_layout, set_memory_rw); - frob_text(&mod->init_layout, set_memory_rw); - frob_rodata(&mod->init_layout, set_memory_rw); + ret = frob_text(&mod->core_layout, set_memory_rw); + if (ret) + return ret; + ret = frob_rodata(&mod->core_layout, set_memory_rw); + if (ret) + return ret; + ret = frob_ro_after_init(&mod->core_layout, set_memory_rw); + if (ret) + return ret; + ret = frob_text(&mod->init_layout, set_memory_rw); + if (ret) + return ret; + ret = frob_rodata(&mod->init_layout, set_memory_rw); + if (ret) + return ret; + + return 0; } -void module_enable_ro(const struct module *mod, bool after_init) +int module_enable_ro(const struct module *mod, bool after_init) { + int ret; if (!rodata_enabled) - return; + return 0; set_vm_flush_reset_perms(mod->core_layout.base); set_vm_flush_reset_perms(mod->init_layout.base); - frob_text(&mod->core_layout, set_memory_ro); + ret = frob_text(&mod->core_layout, set_memory_ro); + if (ret) + return ret; - frob_rodata(&mod->core_layout, set_memory_ro); - frob_text(&mod->init_layout, set_memory_ro); - frob_rodata(&mod->init_layout, set_memory_ro); + ret = frob_rodata(&mod->core_layout, set_memory_ro); + if (ret) + return ret; + ret = frob_text(&mod->init_layout, set_memory_ro); + if (ret) + return ret; + ret = frob_rodata(&mod->init_layout, set_memory_ro); + if (ret) + return ret; - if (after_init) - frob_ro_after_init(&mod->core_layout, set_memory_ro); + if (after_init) { + ret = frob_ro_after_init(&mod->core_layout, set_memory_ro); + if (ret) + return ret; + } + return 0; } -static void module_enable_nx(const struct module *mod) +static int module_enable_nx(const struct module *mod) { - frob_rodata(&mod->core_layout, set_memory_nx); - frob_ro_after_init(&mod->core_layout, set_memory_nx); - frob_writable_data(&mod->core_layout, set_memory_nx); - frob_rodata(&mod->init_layout, set_memory_nx); - frob_writable_data(&mod->init_layout, set_memory_nx); + int ret; + + ret = frob_rodata(&mod->core_layout, set_memory_nx); + if (ret) + return ret; + ret = frob_ro_after_init(&mod->core_layout, set_memory_nx); + if (ret) + return ret; + ret = frob_writable_data(&mod->core_layout, set_memory_nx); + if (ret) + return ret; + ret = frob_rodata(&mod->init_layout, set_memory_nx); + if (ret) + return ret; + ret = frob_writable_data(&mod->init_layout, set_memory_nx); + if (ret) + return ret; + + return 0; } /* Iterate through all modules and set each module's text as RW */ -void set_all_modules_text_rw(void) +int set_all_modules_text_rw(void) { struct module *mod; + int ret; if (!rodata_enabled) - return; + return 0; mutex_lock(&module_mutex); list_for_each_entry_rcu(mod, &modules, list) { if (mod->state == MODULE_STATE_UNFORMED) continue; - frob_text(&mod->core_layout, set_memory_rw); - frob_text(&mod->init_layout, set_memory_rw); + ret = frob_text(&mod->core_layout, set_memory_rw); + if (ret) { + mutex_unlock(&module_mutex); + return ret; + } + ret = frob_text(&mod->init_layout, set_memory_rw); + if (ret) { + mutex_unlock(&module_mutex); + return ret; + } } mutex_unlock(&module_mutex); + return 0; } /* Iterate through all modules and set each module's text as RO */ -void set_all_modules_text_ro(void) +int set_all_modules_text_ro(void) { struct module *mod; + int ret; if (!rodata_enabled) - return; + return 0; mutex_lock(&module_mutex); list_for_each_entry_rcu(mod, &modules, list) { @@ -2017,22 +2068,37 @@ void set_all_modules_text_ro(void) mod->state == MODULE_STATE_GOING) continue; - frob_text(&mod->core_layout, set_memory_ro); - frob_text(&mod->init_layout, set_memory_ro); + ret = frob_text(&mod->core_layout, set_memory_ro); + if (ret) { + mutex_unlock(&module_mutex); + return ret; + } + ret = frob_text(&mod->init_layout, set_memory_ro); + if (ret) { + mutex_unlock(&module_mutex); + return ret; + } } mutex_unlock(&module_mutex); + return 0; } #else /* !CONFIG_STRICT_MODULE_RWX */ -static void module_enable_nx(const struct module *mod) { } +static int module_enable_nx(const struct module *mod) { return 0; } #endif /* CONFIG_STRICT_MODULE_RWX */ -static void module_enable_x(const struct module *mod) +static int module_enable_x(const struct module *mod) { - frob_text(&mod->core_layout, set_memory_x); - frob_text(&mod->init_layout, set_memory_x); + int ret; + ret = frob_text(&mod->core_layout, set_memory_x); + if (ret) + return ret; + ret = frob_text(&mod->init_layout, set_memory_x); + if (ret) + return ret; + return 0; } #else /* !CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ -static void module_enable_nx(const struct module *mod) { } -static void module_enable_x(const struct module *mod) { } +static int module_enable_nx(const struct module *mod) { return 0; } +static int module_enable_x(const struct module *mod) { return 0; } #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ @@ -3534,7 +3600,11 @@ static noinline int do_init_module(struct module *mod) /* Switch to core kallsyms now init is done: kallsyms may be walking! */ rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); #endif - module_enable_ro(mod, true); + ret = module_enable_ro(mod, true); + if (ret) { + mutex_unlock(&module_mutex); + goto fail_free_freeinit; + } mod_tree_remove_init(mod); module_arch_freeing_init(mod); mod->init_layout.base = NULL; @@ -3640,9 +3710,15 @@ static int complete_formation(struct module *mod, struct load_info *info) /* This relies on module_mutex for list integrity. */ module_bug_finalize(info->hdr, info->sechdrs, mod); - module_enable_ro(mod, false); - module_enable_nx(mod); - module_enable_x(mod); + err = module_enable_ro(mod, false); + if (err) + goto out; + err = module_enable_nx(mod); + if (err) + goto out; + err = module_enable_x(mod); + if (err) + goto out; /* Mark state as coming so strong_try_module_get() ignores us, * but kallsyms etc. can see us. */ diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index f9821a3374e9..d4532bb65d1b 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5814,8 +5814,13 @@ void ftrace_module_enable(struct module *mod) * text to read-only, as we now need to set it back to read-write * so that we can modify the text. */ - if (ftrace_start_up) - ftrace_arch_code_modify_prepare(); + if (ftrace_start_up) { + int ret = ftrace_arch_code_modify_prepare(); + if (ret) { + FTRACE_WARN_ON(ret); + goto out_unlock; + } + } do_for_each_ftrace_rec(pg, rec) { int cnt; @@ -5854,8 +5859,10 @@ void ftrace_module_enable(struct module *mod) } while_for_each_ftrace_rec(); out_loop: - if (ftrace_start_up) - ftrace_arch_code_modify_post_process(); + if (ftrace_start_up) { + int ret = ftrace_arch_code_modify_post_process(); + FTRACE_WARN_ON(ret); + } out_unlock: mutex_unlock(&ftrace_lock);
Right now several architectures allow their set_memory_*() family of functions to fail, but callers may not be checking the return values. We need to fix the callers and add the __must_check attribute. They also may not provide any level of atomicity, in the sense that the memory protections may be left incomplete on failure. This issue likely has a few steps on effects architectures[1]: 1)Have all callers of set_memory_*() helpers check the return value. 2)Add __much_check to all set_memory_*() helpers so that new uses do not ignore the return value. 3)Add atomicity to the calls so that the memory protections aren't left in a partial state. Ideally, the failure of set_memory_*() should be passed up the call stack, and callers should examine the failure and deal with it. But currently, some callers just have void return type. We need to fix the callers to handle the return all the way to the top of stack, and it will require a large series of patches to finish all the three steps mentioned above. I start with kernel/module, and will move onto other subsystems. I am not entirely sure about the failure modes for each caller. So I would like to get some comments before I move forward. This single patch is just for fixing the return value of set_memory_*() function in kernel/module, and also the related callers. Any feedback would be greatly appreciated. [1]:https://github.com/KSPP/linux/issues/7 Signed-off-by: Tianlin Li <tli@digitalocean.com> --- arch/arm/kernel/ftrace.c | 8 +- arch/arm64/kernel/ftrace.c | 6 +- arch/nds32/kernel/ftrace.c | 6 +- arch/x86/kernel/ftrace.c | 13 ++- include/linux/module.h | 16 ++-- kernel/livepatch/core.c | 15 +++- kernel/module.c | 170 +++++++++++++++++++++++++++---------- kernel/trace/ftrace.c | 15 +++- 8 files changed, 175 insertions(+), 74 deletions(-)