Re: tracing: user events UAF crash report

[Mathias Krause]

On 23.07.24 16:43, Steven Rostedt wrote:
> On Fri, 19 Jul 2024 22:47:01 +0200
> Mathias Krause <minipli@xxxxxxxxxxxxxx> wrote:
> 
>> 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@xxxxxxxxxxx/

> 
>>  
>>  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