> On Nov 19, 2019, at 11:07 AM, Kees Cook 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 >> --- >> 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 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