Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.