| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240501235044.12fa3297@gandalf.local.home>
Date: Wed, 1 May 2024 23:50:44 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: "Tze-nan Wu (吳澤南)" <Tze-nan.Wu@...iatek.com>
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 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
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;
}
Powered by blists - more mailing lists