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

[Gui Jianfeng]

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.

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