Message-ID: <661f101456506db945ccbd94700a0f47b95f91e5.camel@mediatek.com>
Date: Thu, 2 May 2024 03:10:24 +0000
From: Tze-nan Wu (吳澤南) <Tze-nan.Wu@...iatek.com>
To: "rostedt@...dmis.org" <rostedt@...dmis.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-trace-kernel@...r.kernel.org" <linux-trace-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Cheng-Jui Wang (王正睿)
	<Cheng-Jui.Wang@...iatek.com>, wsd_upstream <wsd_upstream@...iatek.com>,
	Bobule Chang (張弘義) <bobule.chang@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "mhiramat@...nel.org"
	<mhiramat@...nel.org>, "mathieu.desnoyers@...icios.com"
	<mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

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@...dmis.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;
>  }
>  

Powered by blists - more mailing lists