Re: tracing: user events UAF crash report

On 23.07.24 16:43, Steven Rostedt wrote:

On Fri, 19 Jul 2024 22:47:01 +0200
Mathias Krause [off-list ref] wrote:

quoted

Beside the obvious bug, I noticed the following (not fixing the issue,
tho):

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 5d88c184f0fc..687ad0a26458 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c

@@ -112,7 +112,7 @@ static void release_ei(struct kref *ref)
 			entry->release(entry->name, ei->data);
 	}
 
-	call_rcu(&ei->rcu, free_ei_rcu);
+	call_srcu(&eventfs_srcu, &ei->rcu, free_ei_rcu);
 }

This should be fixed too. Care to send a patch for this as well?

Sure, will do.

It use to need RCU but then everything was switched over to SRCU. This was
just leftover.

SRCU usage came up with commit 63940449555e ("eventfs: Implement eventfs
lookup, read, open functions") and following, was further extended just
to get almost completely nuked by [1] earlier this year.

[1]
https://lore.kernel.org/linux-trace-kernel/20240131184918.945345370@goodmis.org/ (local)

quoted

 
 static inline void put_ei(struct eventfs_inode *ei)

@@ -735,7 +735,9 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
 
 	/* Was the parent freed? */
 	if (list_empty(&ei->list)) {
+		mutex_lock(&eventfs_mutex);
 		cleanup_ei(ei);
+		mutex_unlock(&eventfs_mutex);

Why do you think this is needed? The ei is not on the list and has not been
made visible. It was just allocated but the parent it was going to be
attached to is about to be freed.

I have no strong understanding of the code, just reading into what the
context told me the rules should be, which would be on one hand the
following comment...:

/*
 * The eventfs_inode (ei) itself is protected by SRCU. It is released from
 * its parent's list and will have is_freed set (under eventfs_mutex).
 * After the SRCU grace period is over and the last dput() is called
 * the ei is freed.
 */

...and on the other the common pattern, mostly complying to the rule of
first taking the eventfs_mutex, then checking 'is_freed' for a given ei
-- supposedly implying, it can only be set under that very same mutex.

As cleanup_ei() is just a glorified free_ei() which sets ei->is_freed to
1, I was implying the lack of taking eventfs_mutex is a bug. But looking
further for the precondition, getting 'ei' unchained again after it was
put to the parent's children list, I can find eventfs_remove_rec() which
is only ever called under eventfs_mutex and does:

    list_del(&ei->list);
    free_ei(ei);

So you're right and I wasn't paying close enough attention and got
mislead by cleanup_ei() also setting ei->is_freed. But as it should
already be 1 at that point, no bug here.

Thanks,
Mathias