[PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Tze-nan wu]

"tracing_event_file" is at the risk of use-after-free due to the race of
two functions "tracing_open_file_tr" and "synth_event_release".
Specifically, it could be freed by synth_event_release before
tracing_open_file_tr has the opportunity to access its members.

It's easy to reproduced by first executing script 'A' and then script 'B'
in my environment.

  Script 'A':
    echo "hello int aaa" > /sys/kernel/tracing/synthetic_events
    while :
    do
      echo 0 > /sys/kernel/tracing/events/synthetic/hello/enable
    done

  Script 'B':
    echo > /sys/kernel/tracing/synthetic/events

  My environment:
    arm64 + generic and swtag based KASAN both enabled + kernel-6.6.18

KASAN report
==================================================================
BUG: KASAN: slab-use-after-free in tracing_open_file_tr
Read of size 8 at addr 9*ffff********** by task sh/3485
Pointer tag: [9*], memory tag: [fe]

CPU: * PID: 3485 Comm: sh Tainted: ****************
Call trace:
 __hwasan_tag_mismatch
 tracing_open_file_tr
 do_dentry_open
 vfs_open
 path_openat

Freed by task 3490:
 slab_free_freelist_hook
 kmem_cache_free
 event_file_put
 remove_event_file_dir
 __trace_remove_event_call
 trace_remove_event_call
 synth_event_release
 dyn_events_release_all
 synth_events_open

page last allocated via order 0, migratetype Unmovable:
 __slab_alloc
 kmem_cache_alloc
 trace_create_new_event
 trace_add_event_call
 register_synth_event
 __create_synth_event
 create_or_delete_synth_event
 trace_parse_run_command
 synth_events_write
 vfs_write

Based on the assumption that eventfs_inode will persist throughout the
execution of the tracing_open_file_tr function,
we can fix this issue by incrementing the reference count of
trace_event_file once it is assigned to eventfs_inode->data.
The reference count will then be decremented during the release phase of
eventfs_inode.

This ensures that trace_event_file is not prematurely freed while the
tracing_open_file_tr function is being called.

Fixes: bb32500fb9b7 ("tracing: Have trace_event_file have ref counters")
Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
Signed-off-by: Tze-nan wu <Tze-nan.Wu@mediatek.com>
---
BTW, I've also attempted to reproduce the same issue in another
environment (described below).
It's also reproducible but in a lower rate.

another environment:
  x86 + ubuntu + generic kasan enabled + kernel-6.9-rc4

After applying the patch, KASAN no longer reports any issues when
following the same reproduction steps in my arm64 environment. 
However, I am concerned about potential side effects that the patch might introduce.
Additionally, the newly added function "is_entry_event_callback" may not
be reliable in my opinion,
as the callback function used in the comparison could change in future. 
Nonetheless, this is the best solution I can come up with now.

Looking for any suggestion or solution, appreciate.

 fs/tracefs/event_inode.c     | 3 +++
 include/linux/trace_events.h | 4 ++++
 kernel/trace/trace_events.c  | 6 ++++++
 3 files changed, 13 insertions(+)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 894c6ca1e500..fd49c0f88500 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -20,6 +20,7 @@
 #include <linux/workqueue.h>
 #include <linux/security.h>
 #include <linux/tracefs.h>
+#include <linux/trace_events.h>
 #include <linux/kref.h>
 #include <linux/delay.h>
 #include "internal.h"
@@ -90,6 +91,8 @@ static void release_ei(struct kref *ref)
 
 	kfree(ei->entry_attrs);
 	kfree_const(ei->name);
+	if (ei->nr_entries && is_entry_event_callback(ei->entries[0]))
+		event_file_put(ei->data);
 	if (ei->is_events) {
 		rei = get_root_inode(ei);
 		kfree_rcu(rei, ei.rcu);
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 6f9bdfb09d1d..602e87682ee2 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -9,6 +9,7 @@
 #include <linux/hardirq.h>
 #include <linux/perf_event.h>
 #include <linux/tracepoint.h>
+#include <linux/tracefs.h>
 
 struct trace_array;
 struct array_buffer;
@@ -505,6 +506,7 @@ extern struct trace_event_file *trace_get_event_file(const char *instance,
 						     const char *system,
 						     const char *event);
 extern void trace_put_event_file(struct trace_event_file *file);
+bool is_entry_event_callback(struct eventfs_entry entry);
 
 #define MAX_DYNEVENT_CMD_LEN	(2048)
 
@@ -731,6 +733,8 @@ extern void
 event_triggers_post_call(struct trace_event_file *file,
 			 enum event_trigger_type tt);
 
+extern void event_file_put(struct trace_event_file *file);
+
 bool trace_event_ignore_this_pid(struct trace_event_file *trace_file);
 
 bool __trace_trigger_soft_disabled(struct trace_event_file *file);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 52f75c36bbca..de01676b59ff 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2626,6 +2626,7 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 		return -1;
 	}
 
+	event_file_get(file);
 	file->ei = ei;
 
 	ret = event_define_fields(call);
@@ -3420,6 +3421,11 @@ void trace_put_event_file(struct trace_event_file *file)
 }
 EXPORT_SYMBOL_GPL(trace_put_event_file);
 
+bool is_entry_event_callback(struct eventfs_entry entry)
+{
+	return entry.callback == event_callback;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 /* Avoid typos */
-- 
2.18.0

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Steven Rostedt]

On Fri, 26 Apr 2024 15:34:08 +0800
Tze-nan wu <Tze-nan.Wu@mediatek.com> wrote:

> "tracing_event_file" is at the risk of use-after-free due to the race of
> two functions "tracing_open_file_tr" and "synth_event_release".
> Specifically, it could be freed by synth_event_release before
> tracing_open_file_tr has the opportunity to access its members.
> 
> It's easy to reproduced by first executing script 'A' and then script 'B'
> in my environment.
> 
>   Script 'A':
>     echo "hello int aaa" > /sys/kernel/tracing/synthetic_events
>     while :
>     do
>       echo 0 > /sys/kernel/tracing/events/synthetic/hello/enable
>     done
> 
>   Script 'B':
>     echo > /sys/kernel/tracing/synthetic/events

I think you meant:

      echo > /sys/kernel/tracing/synthetic_events

> 
>   My environment:
>     arm64 + generic and swtag based KASAN both enabled + kernel-6.6.18
> 
> KASAN report
> ==================================================================
> BUG: KASAN: slab-use-after-free in tracing_open_file_tr
> Read of size 8 at addr 9*ffff********** by task sh/3485
> Pointer tag: [9*], memory tag: [fe]
> 
> CPU: * PID: 3485 Comm: sh Tainted: ****************
> Call trace:
>  __hwasan_tag_mismatch
>  tracing_open_file_tr
>  do_dentry_open
>  vfs_open
>  path_openat
> 
> Freed by task 3490:
>  slab_free_freelist_hook
>  kmem_cache_free
>  event_file_put
>  remove_event_file_dir
>  __trace_remove_event_call
>  trace_remove_event_call
>  synth_event_release
>  dyn_events_release_all
>  synth_events_open
> 
> page last allocated via order 0, migratetype Unmovable:
>  __slab_alloc
>  kmem_cache_alloc
>  trace_create_new_event
>  trace_add_event_call
>  register_synth_event
>  __create_synth_event
>  create_or_delete_synth_event
>  trace_parse_run_command
>  synth_events_write
>  vfs_write

Thanks for reporting this.

> 
> Based on the assumption that eventfs_inode will persist throughout the
> execution of the tracing_open_file_tr function,
> we can fix this issue by incrementing the reference count of
> trace_event_file once it is assigned to eventfs_inode->data.
> The reference count will then be decremented during the release phase of
> eventfs_inode.
> 
> This ensures that trace_event_file is not prematurely freed while the
> tracing_open_file_tr function is being called.
> 
> Fixes: bb32500fb9b7 ("tracing: Have trace_event_file have ref counters")
> Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> Signed-off-by: Tze-nan wu <Tze-nan.Wu@mediatek.com>
> ---
> BTW, I've also attempted to reproduce the same issue in another
> environment (described below).
> It's also reproducible but in a lower rate.
> 
> another environment:
>   x86 + ubuntu + generic kasan enabled + kernel-6.9-rc4
> 
> After applying the patch, KASAN no longer reports any issues when
> following the same reproduction steps in my arm64 environment. 
> However, I am concerned about potential side effects that the patch might introduce.
> Additionally, the newly added function "is_entry_event_callback" may not
> be reliable in my opinion,
> as the callback function used in the comparison could change in future. 
> Nonetheless, this is the best solution I can come up with now.
> 
> Looking for any suggestion or solution, appreciate.

Yeah, I do not think eventfs should be involved in this. It needs to be
protected at a higher level (in the synthetic/dynamic event code).

I'm just coming back from Japan, and I'll need to take a deeper look at
this after I recover from my jetlag.

Thanks,

-- Steve

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Steven Rostedt]

On Sun, 28 Apr 2024 20:28:37 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > Looking for any suggestion or solution, appreciate.  
> 
> Yeah, I do not think eventfs should be involved in this. It needs to be
> protected at a higher level (in the synthetic/dynamic event code).
> 
> I'm just coming back from Japan, and I'll need to take a deeper look at
> this after I recover from my jetlag.

OK, so I guess the eventfs nodes need an optional release callback. Here's
the right way to do that. I added a "release" function to the passed in
entry array that allows for calling a release function when the
eventfs_inode is freed. Then in code for creating events, I call
event_file_get() on the file being assigned and have the freeing of the
"enable" file have the release function that will call event_file_put() on
that file structure.

Does this fix it for you?

-- Steve

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 894c6ca1e500..dc97c19f9e0a 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -84,10 +84,17 @@ enum {
 static void release_ei(struct kref *ref)
 {
 	struct eventfs_inode *ei = container_of(ref, struct eventfs_inode, kref);
+	const struct eventfs_entry *entry;
 	struct eventfs_root_inode *rei;
 
 	WARN_ON_ONCE(!ei->is_freed);
 
+	for (int i = 0; i < ei->nr_entries; i++) {
+		entry = &ei->entries[i];
+		if (entry->release)
+			entry->release(entry->name, ei->data);
+	}
+
 	kfree(ei->entry_attrs);
 	kfree_const(ei->name);
 	if (ei->is_events) {
diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
index 7a5fe17b6bf9..d03f74658716 100644
--- a/include/linux/tracefs.h
+++ b/include/linux/tracefs.h
@@ -62,6 +62,8 @@ struct eventfs_file;
 typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
 				const struct file_operations **fops);
 
+typedef void (*eventfs_release)(const char *name, void *data);
+
 /**
  * struct eventfs_entry - dynamically created eventfs file call back handler
  * @name:	Then name of the dynamic file in an eventfs directory
@@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
 struct eventfs_entry {
 	const char			*name;
 	eventfs_callback		callback;
+	eventfs_release			release;
 };
 
 struct eventfs_inode;
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 52f75c36bbca..d14c84281f2b 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2552,6 +2552,14 @@ static int event_callback(const char *name, umode_t *mode, void **data,
 	return 0;
 }
 
+/* The file is incremented on creation and freeing the enable file decrements it */
+static void event_release(const char *name, void *data)
+{
+	struct trace_event_file *file = data;
+
+	event_file_put(file);
+}
+
 static int
 event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 {
@@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 		{
 			.name		= "enable",
 			.callback	= event_callback,
+			.release	= event_release,
 		},
 		{
 			.name		= "filter",
@@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 		return ret;
 	}
 
+	/* Gets decremented on freeing of the "enable" file */
+	event_file_get(file);
+
 	return 0;
 }
 

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Tze-nan Wu (吳澤南)]

On Mon, 2024-04-29 at 14:46 -0400, Steven Rostedt wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Sun, 28 Apr 2024 20:28:37 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > Looking for any suggestion or solution, appreciate.  
> > 
> > Yeah, I do not think eventfs should be involved in this. It needs
> to be
> > protected at a higher level (in the synthetic/dynamic event code).
> > 
> > I'm just coming back from Japan, and I'll need to take a deeper
> look at
> > this after I recover from my jetlag.
> 
> OK, so I guess the eventfs nodes need an optional release callback.
> Here's
> the right way to do that. I added a "release" function to the passed
> in
> entry array that allows for calling a release function when the
> eventfs_inode is freed. Then in code for creating events, I call
> event_file_get() on the file being assigned and have the freeing of
> the
> "enable" file have the release function that will call
> event_file_put() on
> that file structure.
> 
> Does this fix it for you?
> 
Sorry for my late reply, I'm testing the patch on my machine now. 
Test will be done in four hours.

There's something I'm worrying about in the patch,
what I'm worrying about is commented in the code below.

/kernel/trace/trace_events.c:
  static int
  event_create_dir(struct eventfs_inode *parent, 
  struct trace_event_file *file) 
  {
        ...
        ...
        ...
        nr_entries = ARRAY_SIZE(event_entries);

        name = trace_event_name(call);

        +event_file_get(file);        // Line A
            ^^^^^^^^^^^^^
        // Should we move the "event_file_get" to here, instead  
        // of calling it at line C?
        // Due to Line B could eventually invoke "event_file_put".
        //   eventfs_create_dir -> free_ei ->put_ei -> kref_put 
        //  -> release_ei -> event_release -> event_file_put
        // Not sure if this is a potential risk? If Line B do call   
        // event_file_put,"event_file_put" will be called prior to
        // "event_file_get", could corrupt the reference of the file.

        ei = eventfs_create_dir(name, e_events,    // Line B 
             event_entries, nr_entries, file);
        if (IS_ERR(ei)) {
                pr_warn("Could not create tracefs '%s' directory\n", 
                name);
                return -1;
        }
        file->ei = ei;

        ret = event_define_fields(call);
        if (ret < 0) {
                pr_warn("Could not initialize trace point events/%s\n",
name);
                return ret;
                   ^^^^^^^^^          
       // Maybe we chould have similar concern if we return here.
       // Due to the event_inode had been created, but we did not call 
       // event_file_get. 
       // Could it lead to some issues in the future while freeing 
       // event_indoe?
        }


        -event_file_get(file);       //Line C
        return 0;
  }

Thanks,
Tze-nan

> -- Steve
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 894c6ca1e500..dc97c19f9e0a 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -84,10 +84,17 @@ enum {
>  static void release_ei(struct kref *ref)
>  {
>  struct eventfs_inode *ei = container_of(ref, struct eventfs_inode,
> kref);
> +const struct eventfs_entry *entry;
>  struct eventfs_root_inode *rei;
>  
>  WARN_ON_ONCE(!ei->is_freed);
>  
> +for (int i = 0; i < ei->nr_entries; i++) {
> +entry = &ei->entries[i];
> +if (entry->release)
> +entry->release(entry->name, ei->data);
> +}
> +
>  kfree(ei->entry_attrs);
>  kfree_const(ei->name);
>  if (ei->is_events) {
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index 7a5fe17b6bf9..d03f74658716 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -62,6 +62,8 @@ struct eventfs_file;
>  typedef int (*eventfs_callback)(const char *name, umode_t *mode,
> void **data,
>  const struct file_operations **fops);
>  
> +typedef void (*eventfs_release)(const char *name, void *data);
> +
>  /**
>   * struct eventfs_entry - dynamically created eventfs file call back
> handler
>   * @name:Then name of the dynamic file in an eventfs directory
> @@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name,
> umode_t *mode, void **data,
>  struct eventfs_entry {
>  const char*name;
>  eventfs_callbackcallback;
> +eventfs_releaserelease;
>  };
>  
>  struct eventfs_inode;
> diff --git a/kernel/trace/trace_events.c
> b/kernel/trace/trace_events.c
> index 52f75c36bbca..d14c84281f2b 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2552,6 +2552,14 @@ static int event_callback(const char *name,
> umode_t *mode, void **data,
>  return 0;
>  }
>  
> +/* The file is incremented on creation and freeing the enable file
> decrements it */
> +static void event_release(const char *name, void *data)
> +{
> +struct trace_event_file *file = data;
> +
> +event_file_put(file);
> +}
> +
>  static int
>  event_create_dir(struct eventfs_inode *parent, struct
> trace_event_file *file)
>  {
> @@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent,
> struct trace_event_file *file)
>  {
>  .name= "enable",
>  .callback= event_callback,
> +.release= event_release,
>  },
>  {
>  .name= "filter",
> @@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent,
> struct trace_event_file *file)
>  return ret;
>  }
>  
> +/* Gets decremented on freeing of the "enable" file */
> +event_file_get(file);
> +
>  return 0;
>  }
>  

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Steven Rostedt]

On Thu, 2 May 2024 03:10:24 +0000
Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com> wrote:

> >   
> Sorry for my late reply, I'm testing the patch on my machine now. 
> Test will be done in four hours.
> 
> There's something I'm worrying about in the patch,
> what I'm worrying about is commented in the code below.
> 
> /kernel/trace/trace_events.c:
>   static int
>   event_create_dir(struct eventfs_inode *parent, 
>   struct trace_event_file *file) 
>   {
>         ...
>         ...
>         ...
>         nr_entries = ARRAY_SIZE(event_entries);
> 
>         name = trace_event_name(call);
> 
>         +event_file_get(file);        // Line A
>             ^^^^^^^^^^^^^
>         // Should we move the "event_file_get" to here, instead  
>         // of calling it at line C?
>         // Due to Line B could eventually invoke "event_file_put".
>         //   eventfs_create_dir -> free_ei ->put_ei -> kref_put 
>         //  -> release_ei -> event_release -> event_file_put
>         // Not sure if this is a potential risk? If Line B do call   
>         // event_file_put,"event_file_put" will be called prior to
>         // "event_file_get", could corrupt the reference of the file.

No, but you do bring up a good point. The release should not be called on
error, but it looks like it possibly can be.

> 
>         ei = eventfs_create_dir(name, e_events,    // Line B 
>              event_entries, nr_entries, file);
>         if (IS_ERR(ei)) {
>                 pr_warn("Could not create tracefs '%s' directory\n", 
>                 name);
>                 return -1;
>         }
>         file->ei = ei;
> 
>         ret = event_define_fields(call);
>         if (ret < 0) {
>                 pr_warn("Could not initialize trace point events/%s\n",
> name);
>                 return ret;
>                    ^^^^^^^^^          
>        // Maybe we chould have similar concern if we return here.
>        // Due to the event_inode had been created, but we did not call 
>        // event_file_get. 
>        // Could it lead to some issues in the future while freeing 
>        // event_indoe?
>         }
> 
> 
>         -event_file_get(file);       //Line C
>         return 0;
>   }

This prevents the release() function from being called on failure of
creating the ei.

Can you try this patch instead?

-- Steve

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 894c6ca1e500..f5510e26f0f6 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -84,10 +84,17 @@ enum {
 static void release_ei(struct kref *ref)
 {
 	struct eventfs_inode *ei = container_of(ref, struct eventfs_inode, kref);
+	const struct eventfs_entry *entry;
 	struct eventfs_root_inode *rei;
 
 	WARN_ON_ONCE(!ei->is_freed);
 
+	for (int i = 0; i < ei->nr_entries; i++) {
+		entry = &ei->entries[i];
+		if (entry->release)
+			entry->release(entry->name, ei->data);
+	}
+
 	kfree(ei->entry_attrs);
 	kfree_const(ei->name);
 	if (ei->is_events) {
@@ -112,6 +119,18 @@ static inline void free_ei(struct eventfs_inode *ei)
 	}
 }
 
+/*
+ * Called when creation of an ei fails, do not call release() functions.
+ */
+static inline void cleanup_ei(struct eventfs_inode *ei)
+{
+	if (ei) {
+		/* Set nr_entries to 0 to prevent release() function being called */
+		ei->nr_entries = 0;
+		free_ei(ei);
+	}
+}
+
 static inline struct eventfs_inode *get_ei(struct eventfs_inode *ei)
 {
 	if (ei)
@@ -734,7 +753,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
 
 	/* Was the parent freed? */
 	if (list_empty(&ei->list)) {
-		free_ei(ei);
+		cleanup_ei(ei);
 		ei = NULL;
 	}
 	return ei;
@@ -835,7 +854,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
 	return ei;
 
  fail:
-	free_ei(ei);
+	cleanup_ei(ei);
 	tracefs_failed_creating(dentry);
 	return ERR_PTR(-ENOMEM);
 }
diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
index 7a5fe17b6bf9..d03f74658716 100644
--- a/include/linux/tracefs.h
+++ b/include/linux/tracefs.h
@@ -62,6 +62,8 @@ struct eventfs_file;
 typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
 				const struct file_operations **fops);
 
+typedef void (*eventfs_release)(const char *name, void *data);
+
 /**
  * struct eventfs_entry - dynamically created eventfs file call back handler
  * @name:	Then name of the dynamic file in an eventfs directory
@@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
 struct eventfs_entry {
 	const char			*name;
 	eventfs_callback		callback;
+	eventfs_release			release;
 };
 
 struct eventfs_inode;
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 52f75c36bbca..6ef29eba90ce 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2552,6 +2552,14 @@ static int event_callback(const char *name, umode_t *mode, void **data,
 	return 0;
 }
 
+/* The file is incremented on creation and freeing the enable file decrements it */
+static void event_release(const char *name, void *data)
+{
+	struct trace_event_file *file = data;
+
+	event_file_put(file);
+}
+
 static int
 event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 {
@@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 		{
 			.name		= "enable",
 			.callback	= event_callback,
+			.release	= event_release,
 		},
 		{
 			.name		= "filter",
@@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 		return ret;
 	}
 
+	/* Gets decremented on freeing of the "enable" file */
+	event_file_get(file);
+
 	return 0;
 }
 

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Tze-nan Wu (吳澤南)]

On Wed, 2024-05-01 at 23:50 -0400, Steven Rostedt wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, 2 May 2024 03:10:24 +0000
> Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com> wrote:
> 
> > >   
> > Sorry for my late reply, I'm testing the patch on my machine now. 
> > Test will be done in four hours.
> > 
> > There's something I'm worrying about in the patch,
> > what I'm worrying about is commented in the code below.
> > 
> > /kernel/trace/trace_events.c:
> >   static int
> >   event_create_dir(struct eventfs_inode *parent, 
> >   struct trace_event_file *file) 
> >   {
> >         ...
> >         ...
> >         ...
> >         nr_entries = ARRAY_SIZE(event_entries);
> > 
> >         name = trace_event_name(call);
> > 
> >         +event_file_get(file);        // Line A
> >             ^^^^^^^^^^^^^
> >         // Should we move the "event_file_get" to here, instead  
> >         // of calling it at line C?
> >         // Due to Line B could eventually invoke "event_file_put".
> >         //   eventfs_create_dir -> free_ei ->put_ei -> kref_put 
> >         //  -> release_ei -> event_release -> event_file_put
> >         // Not sure if this is a potential risk? If Line B do
> call   
> >         // event_file_put,"event_file_put" will be called prior to
> >         // "event_file_get", could corrupt the reference of the
> file.
> 
> No, but you do bring up a good point. The release should not be
> called on
> error, but it looks like it possibly can be.
> 
> > 
> >         ei = eventfs_create_dir(name, e_events,    // Line B 
> >              event_entries, nr_entries, file);
> >         if (IS_ERR(ei)) {
> >                 pr_warn("Could not create tracefs '%s'
> directory\n", 
> >                 name);
> >                 return -1;
> >         }
> >         file->ei = ei;
> > 
> >         ret = event_define_fields(call);
> >         if (ret < 0) {
> >                 pr_warn("Could not initialize trace point
> events/%s\n",
> > name);
> >                 return ret;
> >                    ^^^^^^^^^          
> >        // Maybe we chould have similar concern if we return here.
> >        // Due to the event_inode had been created, but we did not
> call 
> >        // event_file_get. 
> >        // Could it lead to some issues in the future while freeing 
> >        // event_indoe?
> >         }
> > 
> > 
> >         -event_file_get(file);       //Line C
> >         return 0;
> >   }
> 
> This prevents the release() function from being called on failure of
> creating the ei.
> 
> Can you try this patch instead?
> -- Steve
> 
Sure, will reply the mail again after the test finish ASAP.
-- Tze-nan

> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 894c6ca1e500..f5510e26f0f6 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -84,10 +84,17 @@ enum {
>  static void release_ei(struct kref *ref)
>  {
>  struct eventfs_inode *ei = container_of(ref, struct eventfs_inode,
> kref);
> +const struct eventfs_entry *entry;
>  struct eventfs_root_inode *rei;
>  
>  WARN_ON_ONCE(!ei->is_freed);
>  
> +for (int i = 0; i < ei->nr_entries; i++) {
> +entry = &ei->entries[i];
> +if (entry->release)
> +entry->release(entry->name, ei->data);
> +}
> +
>  kfree(ei->entry_attrs);
>  kfree_const(ei->name);
>  if (ei->is_events) {
> @@ -112,6 +119,18 @@ static inline void free_ei(struct eventfs_inode
> *ei)
>  }
>  }
>  
> +/*
> + * Called when creation of an ei fails, do not call release()
> functions.
> + */
> +static inline void cleanup_ei(struct eventfs_inode *ei)
> +{
> +if (ei) {
> +/* Set nr_entries to 0 to prevent release() function being called */
> +ei->nr_entries = 0;
> +free_ei(ei);
> +}
> +}
> +
>  static inline struct eventfs_inode *get_ei(struct eventfs_inode *ei)
>  {
>  if (ei)
> @@ -734,7 +753,7 @@ struct eventfs_inode *eventfs_create_dir(const
> char *name, struct eventfs_inode
>  
>  /* Was the parent freed? */
>  if (list_empty(&ei->list)) {
> -free_ei(ei);
> +cleanup_ei(ei);
>  ei = NULL;
>  }
>  return ei;
> @@ -835,7 +854,7 @@ struct eventfs_inode
> *eventfs_create_events_dir(const char *name, struct dentry
>  return ei;
>  
>   fail:
> -free_ei(ei);
> +cleanup_ei(ei);
>  tracefs_failed_creating(dentry);
>  return ERR_PTR(-ENOMEM);
>  }
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index 7a5fe17b6bf9..d03f74658716 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -62,6 +62,8 @@ struct eventfs_file;
>  typedef int (*eventfs_callback)(const char *name, umode_t *mode,
> void **data,
>  const struct file_operations **fops);
>  
> +typedef void (*eventfs_release)(const char *name, void *data);
> +
>  /**
>   * struct eventfs_entry - dynamically created eventfs file call back
> handler
>   * @name:Then name of the dynamic file in an eventfs directory
> @@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name,
> umode_t *mode, void **data,
>  struct eventfs_entry {
>  const char*name;
>  eventfs_callbackcallback;
> +eventfs_releaserelease;
>  };
>  
>  struct eventfs_inode;
> diff --git a/kernel/trace/trace_events.c
> b/kernel/trace/trace_events.c
> index 52f75c36bbca..6ef29eba90ce 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2552,6 +2552,14 @@ static int event_callback(const char *name,
> umode_t *mode, void **data,
>  return 0;
>  }
>  
> +/* The file is incremented on creation and freeing the enable file
> decrements it */
> +static void event_release(const char *name, void *data)
> +{
> +struct trace_event_file *file = data;
> +
> +event_file_put(file);
> +}
> +
>  static int
>  event_create_dir(struct eventfs_inode *parent, struct
> trace_event_file *file)
>  {
> @@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent,
> struct trace_event_file *file)
>  {
>  .name= "enable",
>  .callback= event_callback,
> +.release= event_release,
>  },
>  {
>  .name= "filter",
> @@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent,
> struct trace_event_file *file)
>  return ret;
>  }
>  
> +/* Gets decremented on freeing of the "enable" file */
> +event_file_get(file);
> +
>  return 0;
>  }
>  

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Tze-nan Wu (吳澤南)]

On Wed, 2024-05-01 at 23:50 -0400, Steven Rostedt wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, 2 May 2024 03:10:24 +0000
> Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com> wrote:
> 
> > >   
> > Sorry for my late reply, I'm testing the patch on my machine now. 
> > Test will be done in four hours.
> > 
> > There's something I'm worrying about in the patch,
> > what I'm worrying about is commented in the code below.
> > 
> > /kernel/trace/trace_events.c:
> >   static int
> >   event_create_dir(struct eventfs_inode *parent, 
> >   struct trace_event_file *file) 
> >   {
> >         ...
> >         ...
> >         ...
> >         nr_entries = ARRAY_SIZE(event_entries);
> > 
> >         name = trace_event_name(call);
> > 
> >         +event_file_get(file);        // Line A
> >             ^^^^^^^^^^^^^
> >         // Should we move the "event_file_get" to here, instead  
> >         // of calling it at line C?
> >         // Due to Line B could eventually invoke "event_file_put".
> >         //   eventfs_create_dir -> free_ei ->put_ei -> kref_put 
> >         //  -> release_ei -> event_release -> event_file_put
> >         // Not sure if this is a potential risk? If Line B do
> call   
> >         // event_file_put,"event_file_put" will be called prior to
> >         // "event_file_get", could corrupt the reference of the
> file.
> 
> No, but you do bring up a good point. The release should not be
> called on
> error, but it looks like it possibly can be.
> 
> > 
> >         ei = eventfs_create_dir(name, e_events,    // Line B 
> >              event_entries, nr_entries, file);
> >         if (IS_ERR(ei)) {
> >                 pr_warn("Could not create tracefs '%s'
> directory\n", 
> >                 name);
> >                 return -1;
> >         }
> >         file->ei = ei;
> > 
> >         ret = event_define_fields(call);
> >         if (ret < 0) {
> >                 pr_warn("Could not initialize trace point
> events/%s\n",
> > name);
> >                 return ret;
> >                    ^^^^^^^^^          
> >        // Maybe we chould have similar concern if we return here.
> >        // Due to the event_inode had been created, but we did not
> call 
> >        // event_file_get. 
> >        // Could it lead to some issues in the future while freeing 
> >        // event_indoe?
> >         }
> > 
> > 
> >         -event_file_get(file);       //Line C
> >         return 0;
> >   }
> 
> This prevents the release() function from being called on failure of
> creating the ei.
> 
> Can you try this patch instead?
> 
> -- Steve
> 
Good news, this patch works, the test has passed, no more Kasan report
in my environment.

my environment:
 arm64 + kasan + swtag based kasan + kernel-6.6.18

Really appreciate, and learn a lot from the patch.

--Tze-nan
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 894c6ca1e500..f5510e26f0f6 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -84,10 +84,17 @@ enum {
>  static void release_ei(struct kref *ref)
>  {
>  struct eventfs_inode *ei = container_of(ref, struct eventfs_inode,
> kref);
> +const struct eventfs_entry *entry;
>  struct eventfs_root_inode *rei;
>  
>  WARN_ON_ONCE(!ei->is_freed);
>  
> +for (int i = 0; i < ei->nr_entries; i++) {
> +entry = &ei->entries[i];
> +if (entry->release)
> +entry->release(entry->name, ei->data);
> +}
> +
>  kfree(ei->entry_attrs);
>  kfree_const(ei->name);
>  if (ei->is_events) {
> @@ -112,6 +119,18 @@ static inline void free_ei(struct eventfs_inode
> *ei)
>  }
>  }
>  
> +/*
> + * Called when creation of an ei fails, do not call release()
> functions.
> + */
> +static inline void cleanup_ei(struct eventfs_inode *ei)
> +{
> +if (ei) {
> +/* Set nr_entries to 0 to prevent release() function being called */
> +ei->nr_entries = 0;
> +free_ei(ei);
> +}
> +}
> +
>  static inline struct eventfs_inode *get_ei(struct eventfs_inode *ei)
>  {
>  if (ei)
> @@ -734,7 +753,7 @@ struct eventfs_inode *eventfs_create_dir(const
> char *name, struct eventfs_inode
>  
>  /* Was the parent freed? */
>  if (list_empty(&ei->list)) {
> -free_ei(ei);
> +cleanup_ei(ei);
>  ei = NULL;
>  }
>  return ei;
> @@ -835,7 +854,7 @@ struct eventfs_inode
> *eventfs_create_events_dir(const char *name, struct dentry
>  return ei;
>  
>   fail:
> -free_ei(ei);
> +cleanup_ei(ei);
>  tracefs_failed_creating(dentry);
>  return ERR_PTR(-ENOMEM);
>  }
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index 7a5fe17b6bf9..d03f74658716 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -62,6 +62,8 @@ struct eventfs_file;
>  typedef int (*eventfs_callback)(const char *name, umode_t *mode,
> void **data,
>  const struct file_operations **fops);
>  
> +typedef void (*eventfs_release)(const char *name, void *data);
> +
>  /**
>   * struct eventfs_entry - dynamically created eventfs file call back
> handler
>   * @name:Then name of the dynamic file in an eventfs directory
> @@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name,
> umode_t *mode, void **data,
>  struct eventfs_entry {
>  const char*name;
>  eventfs_callbackcallback;
> +eventfs_releaserelease;
>  };
>  
>  struct eventfs_inode;
> diff --git a/kernel/trace/trace_events.c
> b/kernel/trace/trace_events.c
> index 52f75c36bbca..6ef29eba90ce 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2552,6 +2552,14 @@ static int event_callback(const char *name,
> umode_t *mode, void **data,
>  return 0;
>  }
>  
> +/* The file is incremented on creation and freeing the enable file
> decrements it */
> +static void event_release(const char *name, void *data)
> +{
> +struct trace_event_file *file = data;
> +
> +event_file_put(file);
> +}
> +
>  static int
>  event_create_dir(struct eventfs_inode *parent, struct
> trace_event_file *file)
>  {
> @@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent,
> struct trace_event_file *file)
>  {
>  .name= "enable",
>  .callback= event_callback,
> +.release= event_release,
>  },
>  {
>  .name= "filter",
> @@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent,
> struct trace_event_file *file)
>  return ret;
>  }
>  
> +/* Gets decremented on freeing of the "enable" file */
> +event_file_get(file);
> +
>  return 0;
>  }
>  

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Steven Rostedt]

On Thu, 2 May 2024 06:49:18 +0000
Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com> wrote:

> Good news, this patch works, the test has passed, no more Kasan report
> in my environment.

Great to hear!

> 
> my environment:
>  arm64 + kasan + swtag based kasan + kernel-6.6.18
> 
> Really appreciate, and learn a lot from the patch.

Can I add:

Tested-by: Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com>

?

-- Steve

Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

[Tze-nan Wu (吳澤南)]

On Thu, 2024-05-02 at 10:09 -0400, Steven Rostedt wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, 2 May 2024 06:49:18 +0000
> Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com> wrote:
> 
> > Good news, this patch works, the test has passed, no more Kasan
> report
> > in my environment.
> 
> Great to hear!
> 
> > 
> > my environment:
> >  arm64 + kasan + swtag based kasan + kernel-6.6.18
> > 
> > Really appreciate, and learn a lot from the patch.
> 
> Can I add:
> 
> Tested-by: Tze-nan Wu (吳澤南) <Tze-nan.Wu@mediatek.com>
> 
> ?
> 
> -- Steve

Certainly, glad to be list as a tester.

-- Tze-nan