tracing: Fix removing instances while reading/writing to their files

!! External Email

From: "Steven Rostedt (Google)" <[email protected]>

Currently when rmdir on an instance is done, eventfs_remove_events_dir() is called and it does a dput on the dentry and then frees the eventfs_inode that represents the events directory.

But there's no protection against a reader reading the top level events directory at the same time and we can get a use after free error. Instead, use the dput() associated to the dentry to also free the eventfs_inode associated to the events directory, as that will get called when the last reference to the directory is released.

Link https://lore.kernel.org/all/[email protected]/ open here

Cc Ajay Kaher <[email protected]>

Fixes 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")

Reported-by Zheng Yejian <[email protected]>

Signed-off-by Steven Rostedt (Google) <[email protected]>

---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/[email protected]
- Removed left over "ei" variable (kernel test robot)

fs/tracefs/event_inode.c | 17 ++++++++++++-----
fs/tracefs/inode.c       |  2 +-
fs/tracefs/internal.h    |  5 +++--
3 files changed, 16 insertions(+), 8 deletions(-)

fs/tracefs/event_inode.c +12 -5

index fa1a1679a886..609ccb5b7cfc 100644
@@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
 
 /**
 * eventfs_set_ef_status_free - set the ef->status to free
+ * @ti: the tracefs_inode of the dentry
 * @dentry: dentry who's status to be freed
 *
 * eventfs_set_ef_status_free will be called if no more
 * references remain
 */
-void eventfs_set_ef_status_free(struct dentry *dentry)
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
 {
       struct tracefs_inode *ti_parent;
+       struct eventfs_inode *ei;
       struct eventfs_file *ef;
 
       mutex_lock(&eventfs_mutex);
+
+       /* The top level events directory may be freed by this */
+       if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
+               ei = ti->private;
+               kfree(ei);
+               goto out;
+       }
+
       ti_parent = get_tracefs(dentry->d_parent->d_inode);
       if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
               goto out;

@@ -510,7 +520,7 @@ struct dentry *eventfs_create_events_dir(const char *name,
       INIT_LIST_HEAD(&ei->e_top_files);
 
       ti = get_tracefs(inode);
-       ti->flags |= TRACEFS_EVENT_INODE;
+       ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
       ti->private = ei;
 
       inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;

@@ -806,7 +816,6 @@ void eventfs_remove(struct eventfs_file *ef)
 void eventfs_remove_events_dir(struct dentry *dentry)
 {
       struct tracefs_inode *ti;
-       struct eventfs_inode *ei;
 
       if (!dentry || !dentry->d_inode)
               return;

@@ -815,8 +824,6 @@ void eventfs_remove_events_dir(struct dentry *dentry)
       if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
               return;
 
-       ei = ti->private;
       d_invalidate(dentry);
       dput(dentry);
-       kfree(ei);
 }

fs/tracefs/inode.c +1 -1

index 3b8dd938b1c8..891653ba9cf3 100644
@@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)
 
       ti = get_tracefs(inode);
       if (ti && ti->flags & TRACEFS_EVENT_INODE)
-               eventfs_set_ef_status_free(dentry);
+               eventfs_set_ef_status_free(ti, dentry);
       iput(inode);
 }
 

fs/tracefs/internal.h +3 -3

index 69c2b1d87c46..4f2e49e2197b 100644
@@ -3,7 +3,8 @@
 #define _TRACEFS_INTERNAL_H
 
 enum {
-       TRACEFS_EVENT_INODE     = BIT(1),
+       TRACEFS_EVENT_INODE             = BIT(1),
+       TRACEFS_EVENT_TOP_INODE         = BIT(2),
 };
 
 struct tracefs_inode {

@@ -24,6 +25,6 @@ struct inode *tracefs_get_inode(struct super_block *sb);
 struct dentry *eventfs_start_creating(const char *name, struct dentry *parent);
 struct dentry *eventfs_failed_creating(struct dentry *dentry);
 struct dentry *eventfs_end_creating(struct dentry *dentry);
-void eventfs_set_ef_status_free(struct dentry *dentry);
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry);
 
 #endif /* _TRACEFS_INTERNAL_H */
--

2.40.1

!! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.

Nice catch Steve. Changes looks good to me.

Would like to know how did you map the backtrace with use-after-free of eventfs_inode.

Thinking if same problem could happen for sub folder/files of eventfs as free_ef() may get call earlier then dput().

Thinking if same problem could happen for sub folder/files of eventfs as free_ef() may get call earlier then dput().

From: "Steven Rostedt (Google)" <[email protected]>

Currently when rmdir on an instance is done, eventfs_remove_events_dir() is called and it does a dput on the dentry and then frees the eventfs_inode that represents the events directory.

But there's no protection against a reader reading the top level events directory at the same time and we can get a use after free error. Instead, use the dput() associated to the dentry to also free the eventfs_inode associated to the events directory, as that will get called when the last reference to the directory is released.

Link https://lore.kernel.org/all/[email protected]/ open here

Cc Ajay Kaher <[email protected]>

Fixes 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")

Reported-by Zheng Yejian <[email protected]>

Signed-off-by Steven Rostedt (Google) <[email protected]>

---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/[email protected]
- Removed left over "ei" variable (kernel test robot)

 fs/tracefs/event_inode.c | 17 ++++++++++++-----
 fs/tracefs/inode.c       |  2 +-
 fs/tracefs/internal.h    |  5 +++--
 3 files changed, 16 insertions(+), 8 deletions(-)

fs/tracefs/event_inode.c +2 -1

index fa1a1679a886..609ccb5b7cfc 100644
@@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
 
 /**
  * eventfs_set_ef_status_free - set the ef->status to free
+ * @ti: the tracefs_inode of the dentry
  * @dentry: dentry who's status to be freed
  *
  * eventfs_set_ef_status_free will be called if no more
  * references remain
  */
-void eventfs_set_ef_status_free(struct dentry *dentry)
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
 {

fs/tracefs/event_inode.c +1 -4

@@ -510,7 +520,7 @@ struct dentry *eventfs_create_events_dir(const char *name,
 	INIT_LIST_HEAD(&ei->e_top_files);
 
 	ti = get_tracefs(inode);
-	ti->flags |= TRACEFS_EVENT_INODE;
+	ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
 	ti->private = ei;
 
 	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;

@@ -806,7 +816,6 @@ void eventfs_remove(struct eventfs_file *ef)
 void eventfs_remove_events_dir(struct dentry *dentry)
 {
 	struct tracefs_inode *ti;
-	struct eventfs_inode *ei;
 
 	if (!dentry || !dentry->d_inode)
 		return;

@@ -815,8 +824,6 @@ void eventfs_remove_events_dir(struct dentry *dentry)
 	if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
 		return;
 
-	ei = ti->private;
 	d_invalidate(dentry);
 	dput(dentry);
-	kfree(ei);
 }

fs/tracefs/inode.c +1 -1

index 3b8dd938b1c8..891653ba9cf3 100644
@@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)
 
 	ti = get_tracefs(inode);
 	if (ti && ti->flags & TRACEFS_EVENT_INODE)
-		eventfs_set_ef_status_free(dentry);
+		eventfs_set_ef_status_free(ti, dentry);
 	iput(inode);
 }
 

fs/tracefs/internal.h +3 -2

index 69c2b1d87c46..4f2e49e2197b 100644
@@ -3,7 +3,8 @@
 #define _TRACEFS_INTERNAL_H
 
 enum {
-	TRACEFS_EVENT_INODE     = BIT(1),
+	TRACEFS_EVENT_INODE		= BIT(1),
+	TRACEFS_EVENT_TOP_INODE		= BIT(2),
 };
 
 struct tracefs_inode {

@@ -24,6 +25,6 @@ struct inode *tracefs_get_inode(struct super_block *sb);
 struct dentry *eventfs_start_creating(const char *name, struct dentry *parent);
 struct dentry *eventfs_failed_creating(struct dentry *dentry);
 struct dentry *eventfs_end_creating(struct dentry *dentry);
-void eventfs_set_ef_status_free(struct dentry *dentry);
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry);
 
 #endif /* _TRACEFS_INTERNAL_H */

Signature · 1 line

2.40.1

Hi,

Quoted 48 lines · On Wed, 06 Sep 2023 22:47:11 -0400 Steven Rostedt <[email protected]> wrote:

From: "Steven Rostedt (Google)" <[email protected]>

Currently when rmdir on an instance is done, eventfs_remove_events_dir() is called and it does a dput on the dentry and then frees the eventfs_inode that represents the events directory.

But there's no protection against a reader reading the top level events directory at the same time and we can get a use after free error. Instead, use the dput() associated to the dentry to also free the eventfs_inode associated to the events directory, as that will get called when the last reference to the directory is released.

Link https://lore.kernel.org/all/[email protected]/ open here

Cc Ajay Kaher <[email protected]>

Fixes 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")

Reported-by Zheng Yejian <[email protected]>

Signed-off-by Steven Rostedt (Google) <[email protected]>

---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/[email protected]
- Removed left over "ei" variable (kernel test robot)

 fs/tracefs/event_inode.c | 17 ++++++++++++-----
 fs/tracefs/inode.c       |  2 +-
 fs/tracefs/internal.h    |  5 +++--
 3 files changed, 16 insertions(+), 8 deletions(-)

fs/tracefs/event_inode.c +3 -1

index fa1a1679a886..609ccb5b7cfc 100644
@@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
 
 /**
  * eventfs_set_ef_status_free - set the ef->status to free
+ * @ti: the tracefs_inode of the dentry
  * @dentry: dentry who's status to be freed
  *
  * eventfs_set_ef_status_free will be called if no more
  * references remain
  */
-void eventfs_set_ef_status_free(struct dentry *dentry)
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
 {
 	struct tracefs_inode *ti_parent;
+	struct eventfs_inode *ei;
 	struct eventfs_file *ef;
 

fs/tracefs/event_inode.c static struct dentry *create_dir(const char *name, struct dentry *parent, void * · line 205

Quoted 6 lines

fs/tracefs/event_inode.c +5 -0

 	mutex_lock(&eventfs_mutex);
+
+	/* The top level events directory may be freed by this */
+	if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
+		ei = ti->private;
+		kfree(ei);  

Don't we need to clear 'ti->private' here to avoid accessing (or double free) ti->private somewhare?

fs/tracefs/event_inode.c static struct dentry *create_dir(const char *name, struct dentry *parent, void * · line 211

Quoted 7 lines

fs/tracefs/event_inode.c +3 -0

+		goto out;
+	}
+
 	ti_parent = get_tracefs(dentry->d_parent->d_inode);
 	if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
 		goto out;  

Here, I guess this "!(ti_parent->flags & TRACEFS_EVENT_INODE)" means this inode is TRACEFS_EVENT_TOP_INODE, so this check may not be needed, is this correct?

It appears that the dynamic code of eventfs has caused a race window to open up a bit more and showed that several files were not protected by the trace array ref count. This means that a task could open one of the files in an instance, remove the instance, and still be able to read or write to that file. That access will then cause a "use-after-free" bug.

Close those holes!

Also, fix a left over unused variable in the eventfs dput fix.

Steven Rostedt (Google) (6):
tracefs/eventfs: Use dput to free the toplevel events directory
tracing: Increase trace array ref count on enable and filter files
tracing: Have tracing_max_latency inc the trace array ref count
tracing: Have current_trace inc the trace array ref count
tracing: Have option files inc the trace array ref count
tracing: Have event inject files inc the trace array ref count

----

 fs/tracefs/event_inode.c           | 17 +++++++---
 fs/tracefs/inode.c                 |  2 +-
 fs/tracefs/internal.h              |  5 +--
 kernel/trace/trace.c               | 68 ++++++++++++++++++++++++++++++++++----
 kernel/trace/trace.h               |  2 ++
 kernel/trace/trace_events.c        |  6 ++--
 kernel/trace/trace_events_inject.c |  3 +-
 7 files changed, 85 insertions(+), 18 deletions(-)

This patch set applied on top of Linux next master branch and tested selftests ftrace tests [1]. The test run to complete and not found any crashes.

Tested-by Linux Kernel Functional Testing <[email protected]>

Tested-by Naresh Kamboju <[email protected]>