Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr
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 + > Tze-nan Wu (吳澤南) 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 (吳澤南) > > ? > > -- Steve Certainly, glad to be list as a tester. -- Tze-nan
Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr
On Thu, 2 May 2024 06:49:18 + Tze-nan Wu (吳澤南) 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 (吳澤南) ? -- Steve
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 +
> Tze-nan Wu (吳澤南) 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 = >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(>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
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 +
> Tze-nan Wu (吳澤南) 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 = >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(>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,
>
Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr
On Thu, 2 May 2024 03:10:24 +
Tze-nan Wu (吳澤南) 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 = >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(>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_release release;
};
struct eventfs_inode;
diff --git a/kernel/trace/trace_events.c
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 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 = >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;
> +
>
Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr
On Sun, 28 Apr 2024 20:28:37 -0400
Steven Rostedt 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 = >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_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
On Fri, 26 Apr 2024 15:34:08 +0800
Tze-nan wu 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*** 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
> Signed-off-by: Cheng-Jui Wang
> Signed-off-by: Tze-nan wu
> ---
> 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
