[ltt-dev] [PATCH v2] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> >> Hi Mathieu,
> >>
> >> Calling get_ltt_root() and put_ltt_root() pair may induce invalid pointer
> >> derefference. Consider the following scenario:
> >>
> >>          CPU 0                   CPU 1
> >>   ----------------------  ----------------------
> >> 1  root = get_ltt_root()   
> >> 2                          root = get_ltt_root()
> >> 3                          put_ltt_root() //global root is freed
> >> 4  using root(crash here)
> >>
> >> Here is the fix for this issue. Thanks a lot to Zhaolei to point this out
> >> in offline.
> >>
> >> This patch assumes that the *ltt root remove* patch has been applied.
> >>
> >> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
> >> ---
> >>  include/linux/ltt-core.h  |    3 ++-
> >>  ltt/ltt-core.c            |   25 +++++++++++++++++++++----
> >>  ltt/ltt-relay.c           |   10 +++++++++-
> >>  ltt/ltt-userspace-event.c |    3 +++
> >>  4 files changed, 35 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
> >> index 28394cb..786c2b1 100644
> >> --- a/include/linux/ltt-core.h
> >> +++ b/include/linux/ltt-core.h
> >> @@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
> >>  /*
> >>   * get dentry of ltt's root dir
> >>   */
> >> -void put_ltt_root(void);
> >>  struct dentry *get_ltt_root(void);
> >>  
> >> +void put_ltt_root(void);
> >> +
> >>  /* Keep track of trap nesting inside LTT */
> >>  DECLARE_PER_CPU(unsigned int, ltt_nesting);
> >>  
> >> diff --git a/ltt/ltt-core.c b/ltt/ltt-core.c
> >> index 314750b..03b81ca 100644
> >> --- a/ltt/ltt-core.c
> >> +++ b/ltt/ltt-core.c
> >> @@ -10,6 +10,7 @@
> >>  #include <linux/percpu.h>
> >>  #include <linux/module.h>
> >>  #include <linux/debugfs.h>
> >> +#include <linux/kref.h>
> >>  
> >>  /* Traces structures */
> >>  struct ltt_traces ltt_traces = {
> >> @@ -23,12 +24,22 @@ static DEFINE_MUTEX(ltt_traces_mutex);
> >>  
> >>  /* dentry of ltt's root dir */
> >>  static struct dentry *ltt_root_dentry;
> >> +static struct kref ltt_root_kref = {
> >> +	.refcount = ATOMIC_INIT(0),
> >> +};
> >> +
> >> +static void ltt_root_release(struct kref *ref)
> >> +{
> >> +	return;
> >> +}
> >>  
> >>  void put_ltt_root(void)
> >>  {
> >> -	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
> >> -		debugfs_remove(ltt_root_dentry);
> >> -		ltt_root_dentry = NULL;
> >> +	if (ltt_root_dentry) {
> >> +		if (kref_put(&ltt_root_kref, ltt_root_release)) {
> >> +			debugfs_remove(ltt_root_dentry);
> >> +			ltt_root_dentry = NULL;
> > 
> > Those two should go in ltt_root_release. See other (wide) use of
> > kref_get/kref_put in the LTT code as example.
> > 
> > Mathieu
> > 
> >> +		}
> >>  	}
> >>  }
> >>  EXPORT_SYMBOL_GPL(put_ltt_root);
> >> @@ -37,9 +48,15 @@ struct dentry *get_ltt_root(void)
> >>  {
> >>  	if (!ltt_root_dentry) {
> >>  		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
> >> -		if (!ltt_root_dentry)
> >> +		if (!ltt_root_dentry) {
> >>  			printk(KERN_ERR "LTT : create ltt root dir failed\n");
> >> +			goto out;
> >> +		}
> >> +		kref_init(&ltt_root_kref);
> >> +		goto out;
> >>  	}
> >> +	kref_get(&ltt_root_kref);
> >> +out:
> >>  	return ltt_root_dentry;
> >>  }
> >>  EXPORT_SYMBOL_GPL(get_ltt_root);
> >> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
> >> index 7c544f4..8378802 100644
> >> --- a/ltt/ltt-relay.c
> >> +++ b/ltt/ltt-relay.c
> >> @@ -833,8 +833,16 @@ end:
> >>  
> >>  static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
> >>  {
> >> +	struct dentry *ltt_root_dentry;
> >> +
> >> +	ltt_root_dentry = get_ltt_root();
> >> +	if (!ltt_root_dentry)
> >> +		return ENOENT;
> >> +
> >>  	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
> >> -			get_ltt_root());
> >> +			ltt_root_dentry);
> >> +	put_ltt_root();
> >> +	ltt_root_dentry = NULL;
> > 
> > If ltt_root_dentry is local to the function, I don't see real value in
> > setting it to null ?
> 
>   Ok, will remove.
> 
> > 
> > You should also modifify ltt-relay-locked, because it has almost the
> > exact same code as ltt-relay.c.
> 
>   ltt-relay-locked doesn't use get_ltt_root(), it has its own root directory
>   called "ltt-locked", so i don't see why ltt-relay-locked should be modified.
> 

Yes, you are right.

Mathieu

> > 
> > Thanks,
> > 
> > Mathieu
> > 
> >>  	if (new_trace->dentry.trace_root == NULL) {
> >>  		printk(KERN_ERR "LTT : Trace directory name %s already taken\n",
> >>  				new_trace->trace_name);
> >> diff --git a/ltt/ltt-userspace-event.c b/ltt/ltt-userspace-event.c
> >> index a48f336..487516b 100644
> >> --- a/ltt/ltt-userspace-event.c
> >> +++ b/ltt/ltt-userspace-event.c
> >> @@ -110,7 +110,10 @@ static int __init ltt_userspace_init(void)
> >>  		err = -EPERM;
> >>  		goto err_no_file;
> >>  	}
> >> +
> >> +	return err;
> >>  err_no_file:
> >> +	put_ltt_root();
> >>  err_no_root:
> >>  	return err;
> >>  }
> >> -- 
> >> 1.5.4.rc3
> >>
> >>
> >> _______________________________________________
> >> ltt-dev mailing list
> >> ltt-dev at lists.casi.polymtl.ca
> >> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> >>
> > 
> 
> -- 
> Regards
> Gui Jianfeng
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68