Message-ID: <aac0a4cc0a12fc9593ab7de6c34836757e6814f3.camel@mediatek.com>
Date: Thu, 2 May 2024 06:49:18 +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 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@...iatek.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;
>  }
>  

Powered by blists - more mailing lists