[RFC] kernel/module: have the callers of set_memory_*() check the return value
diff mbox series

Message ID 20191119155149.20396-1-tli@digitalocean.com
State New
Headers show
Series
  • [RFC] kernel/module: have the callers of set_memory_*() check the return value
Related show

Commit Message

Tianlin Li Nov. 19, 2019, 3:51 p.m. UTC
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(-)

Comments

Kees Cook Nov. 19, 2019, 5:07 p.m. UTC | #1
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!
Peter Zijlstra Nov. 19, 2019, 9:38 p.m. UTC | #2
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().
Peter Zijlstra Nov. 19, 2019, 9:43 p.m. UTC | #3
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
Tianlin Li Nov. 19, 2019, 9:51 p.m. UTC | #4
> 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
Tianlin Li Nov. 19, 2019, 9:54 p.m. UTC | #5
> 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.
Borislav Petkov Nov. 20, 2019, 9:58 a.m. UTC | #6
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.
Tianlin Li Nov. 20, 2019, 4:19 p.m. UTC | #7
> 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>

Patch
diff mbox series

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);