Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1505071 > unrolled thread

[RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ?

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ? Aaron Tomlin <atomlin@redhat.com> - 2016-10-20 18:20 +0200
    [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code Aaron Tomlin <atomlin@redhat.com> - 2016-10-20 18:20 +0200
      Re: [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code Rusty Russell <rusty@rustcorp.com.au> - 2016-10-26 03:40 +0200
    [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too Aaron Tomlin <atomlin@redhat.com> - 2016-10-20 18:20 +0200
      Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too Rusty Russell <rusty@rustcorp.com.au> - 2016-10-26 03:40 +0200
        Re: [RFC PATCH 2/2] module: When modifying a module's text ignore  modules which are going away too Steven Rostedt <rostedt@goodmis.org> - 2016-10-26 14:10 +0200

#1505071 — [RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ?

I think there is a race (albeit a hard-to-hit one) between load_module()
error handling and kprobe registration which could cause a kernel page to
become read-only, panic due to protection fault.

In short, the protection that gets applied [at the bug_cleanup label] can be
overridden by another CPU when executing set_all_modules_text_ro().
Therefore creating the possibility for the kprobe registration code path to
touch a [formed] module that is being deallocated. Consequently we could
free a mapped page, that is not 'writable'. The same page, when later
accessed, will result in a page fault which cannot be handled. Below is an
attempt to illustrate the race. Please note we assume that:

      - kprobe uses ftrace

      - parse_args() or mod_sysfs_setup() would have to fail

      - CPU Y and X do not attempt to load the same module

      - CPU Y would have to sneak in *after* CPU X called the two 'unset'
	functions but before CPU X removes the module from the list of all
	modules

       CPU X
   ...
     load_module
       // Unknown/invalid module
       // parameter specified ...
       after_dashes = parse_args(...)
       if (IS_ERR(after_dashes))
         err = PTR_ERR(after_dashes)
         goto coming_cleanup:
       ...
      bug_cleanup:

       module_disable_ro(mod)
       module_disable_nx(mod)
       ...
       // set_all_modules_text_ro() on CPU Y sneaks in here <-----.
       // and overrides the effects of the previous 'unset'       |
       ...                                                        |
       list_del_rcu(&mod->list)                                   |
                                                                  |
                                                                  |
       CPU Y                                                      |
   ...                                                            |
     sys_finit_module                                             |
       load_module                                                |
         do_init_module                                           |
	   do_one_initcall                                        |
	     // mod->init                                         |
	     kprobe_example_init                                  |
	       register_kprobe                                    |
	         arm_kprobe                                       |
		   // kprobe uses ftrace                          |
		   arm_kprobe_ftrace                              |
		     register_ftrace_function                     |
		       ftrace_startup                             |
		         ftrace_startup_enable                    |
			   ftrace_run_update_code                 |
			     ftrace_arch_code_modify_post_process |
			     {                                    |
			       //                                 |
			       // Set all [formed] module's       |
                               // core and init pages as          |
                               // read-only under                 |
			       // module_mutex ...                |
			       //                                 |
	                       set_all_modules_text_ro() ---------'
			     }



The following patches (I hope) is an attempt to address this theoretical
race.  Please let me know your thoughts.


Aaron Tomlin (2):
  module: Ensure a module's state is set accordingly during module
    coming cleanup code
  module: When modifying a module's text ignore modules which are going
    away too

 kernel/module.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1505074 — [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code

In load_module() in the event of an error, for e.g. unknown module
parameter(s) specified we go to perform some module coming clean up
operations. At this point the module is still in a "formed" state
when it is actually going away.

This patch updates the module's state accordingly to ensure anyone on the
module_notify_list waiting for a module going away notification will be
notified accordingly.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/module.c b/kernel/module.c
index f57dd63..ff93ab8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3708,6 +3708,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
  sysfs_cleanup:
 	mod_sysfs_teardown(mod);
  coming_cleanup:
+	mod->state = MODULE_STATE_GOING;
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1508746 — Re: [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code

Aaron Tomlin <atomlin@redhat.com> writes:
> In load_module() in the event of an error, for e.g. unknown module
> parameter(s) specified we go to perform some module coming clean up
> operations. At this point the module is still in a "formed" state
> when it is actually going away.
>
> This patch updates the module's state accordingly to ensure anyone on the
> module_notify_list waiting for a module going away notification will be
> notified accordingly.

I recall a similar proposal before.

I've audited all the subscribers to check they didn't look at
mod->state; they seem OK.

We actually do this in the init-failed path, so this should be OK.

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63..ff93ab8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3708,6 +3708,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>   sysfs_cleanup:
>  	mod_sysfs_teardown(mod);
>   coming_cleanup:
> +	mod->state = MODULE_STATE_GOING;
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);
> -- 
> 2.5.5

[toc] | [prev] | [next] | [standalone]

#1505075 — [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too

By default, during the access permission modification of a module's core
and init pages, we only ignore modules that are malformed. There is no
reason not to extend this to modules which are going away too.

This patch makes both set_all_modules_text_rw() and
set_all_modules_text_ro() skip modules which are going away too.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index ff93ab8..09c386b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(void)
 
 	mutex_lock(&module_mutex);
 	list_for_each_entry_rcu(mod, &modules, list) {
-		if (mod->state == MODULE_STATE_UNFORMED)
+		if (mod->state == MODULE_STATE_UNFORMED ||
+			mod->state == MODULE_STATE_GOING)
 			continue;
 
 		frob_text(&mod->core_layout, set_memory_rw);
@@ -1969,7 +1970,8 @@ void set_all_modules_text_ro(void)
 
 	mutex_lock(&module_mutex);
 	list_for_each_entry_rcu(mod, &modules, list) {
-		if (mod->state == MODULE_STATE_UNFORMED)
+		if (mod->state == MODULE_STATE_UNFORMED ||
+			mod->state == MODULE_STATE_GOING)
 			continue;
 
 		frob_text(&mod->core_layout, set_memory_ro);
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1508747 — Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too

Aaron Tomlin <atomlin@redhat.com> writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. There is no
> reason not to extend this to modules which are going away too.

Well, it depends on all the callers (ie. ftrace): is that also ignoring
modules which are going away?

Otherwise, we set MODULE_STATE_GOING, ftrace walks all the modules and
this one is still RO...

Thanks,
Rusty.

> This patch makes both set_all_modules_text_rw() and
> set_all_modules_text_ro() skip modules which are going away too.
>
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  kernel/module.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..09c386b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(void)
>  
>  	mutex_lock(&module_mutex);
>  	list_for_each_entry_rcu(mod, &modules, list) {
> -		if (mod->state == MODULE_STATE_UNFORMED)
> +		if (mod->state == MODULE_STATE_UNFORMED ||
> +			mod->state == MODULE_STATE_GOING)
>  			continue;
>  
>  		frob_text(&mod->core_layout, set_memory_rw);
> @@ -1969,7 +1970,8 @@ void set_all_modules_text_ro(void)
>  
>  	mutex_lock(&module_mutex);
>  	list_for_each_entry_rcu(mod, &modules, list) {
> -		if (mod->state == MODULE_STATE_UNFORMED)
> +		if (mod->state == MODULE_STATE_UNFORMED ||
> +			mod->state == MODULE_STATE_GOING)
>  			continue;
>  
>  		frob_text(&mod->core_layout, set_memory_ro);
> -- 
> 2.5.5

[toc] | [prev] | [next] | [standalone]

#1509115 — Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too

On Wed, 26 Oct 2016 11:35:18 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Aaron Tomlin <atomlin@redhat.com> writes:
> > By default, during the access permission modification of a module's core
> > and init pages, we only ignore modules that are malformed. There is no
> > reason not to extend this to modules which are going away too.  
> 
> Well, it depends on all the callers (ie. ftrace): is that also ignoring
> modules which are going away?
> 
> Otherwise, we set MODULE_STATE_GOING, ftrace walks all the modules and
> this one is still RO...
> 

Actually, looking into this more, you are correct. There's a
possibility in enabling ftrace after the module is about to go but
before ftrace_release_mod() is called (which will remove the module
text from the ftrace function list).

I don't see any reason for not allowing set_all_modules_text_rw() from
being called if a module is going. If a module is going, shouldn't its
text be rw anyway?

Perhaps just preventing it from turning into ro will be sufficient. And
remove the check from set_all_modules_text_rw().

-- Steve


> Thanks,
> Rusty.
> 
> > This patch makes both set_all_modules_text_rw() and
> > set_all_modules_text_ro() skip modules which are going away too.
> >
> > Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> > ---
> >  kernel/module.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index ff93ab8..09c386b 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(void)
> >  
> >  	mutex_lock(&module_mutex);
> >  	list_for_each_entry_rcu(mod, &modules, list) {
> > -		if (mod->state == MODULE_STATE_UNFORMED)
> > +		if (mod->state == MODULE_STATE_UNFORMED ||
> > +			mod->state == MODULE_STATE_GOING)
> >  			continue;
> >  
> >  		frob_text(&mod->core_layout, set_memory_rw);
> > @@ -1969,7 +1970,8 @@ void set_all_modules_text_ro(void)
> >  
> >  	mutex_lock(&module_mutex);
> >  	list_for_each_entry_rcu(mod, &modules, list) {
> > -		if (mod->state == MODULE_STATE_UNFORMED)
> > +		if (mod->state == MODULE_STATE_UNFORMED ||
> > +			mod->state == MODULE_STATE_GOING)
> >  			continue;
> >  
> >  		frob_text(&mod->core_layout, set_memory_ro);
> > -- 
> > 2.5.5  

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web