From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Thu, 26 Feb 2009 16:36:15 -0500 Subject: [ltt-dev] [PATCH v2] lttng: Fix potential invalid pointer derefference In-Reply-To: <49A353C3.8000000@cn.fujitsu.com> References: <49A237E5.4010001@cn.fujitsu.com> <49A26C0B.3010005@cn.fujitsu.com> <20090223143709.GA27239@Krystal> <49A353C3.8000000@cn.fujitsu.com> Message-ID: <20090226213615.GA20856@Krystal> * 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 > >> --- > >> 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 > >> #include > >> #include > >> +#include > >> > >> /* 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(<t_root_dentry->d_subdirs)) { > >> - debugfs_remove(ltt_root_dentry); > >> - ltt_root_dentry = NULL; > >> + if (ltt_root_dentry) { > >> + if (kref_put(<t_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(<t_root_kref); > >> + goto out; > >> } > >> + kref_get(<t_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