[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <201911190849.131691D@keescook>
Date: Tue, 19 Nov 2019 09:07:34 -0800
From: Kees Cook <keescook@...omium.org>
To: Tianlin Li <tli@...italocean.com>
Cc: kernel-hardening@...ts.openwall.com,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
Russell King <linux@...linux.org.uk>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Greentime Hu <green.hu@...il.com>,
Vincent Chen <deanbo422@...il.com>,
Thomas Gleixner <tglx@...utronix.de>,
Borislav Petkov <bp@...en8.de>, "H . Peter Anvin" <hpa@...or.com>,
x86@...nel.org, Jessica Yu <jeyu@...nel.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Jiri Kosina <jikos@...nel.org>, Miroslav Benes <mbenes@...e.cz>,
Petr Mladek <pmladek@...e.com>,
Joe Lawrence <joe.lawrence@...hat.com>
Subject: Re: [RFC PATCH] kernel/module: have the callers of set_memory_*()
check the return value
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@...italocean.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@...r.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!
--
Kees Cook
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
