linux-kernel - Re: tracing: user events UAF crash report

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <10dd9e0f-fb2d-4667-8fba-171040690055@grsecurity.net>
Date: Tue, 23 Jul 2024 22:54:49 +0200
From: Mathias Krause <minipli@...ecurity.net>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
 Ajay Kaher <ajay.kaher@...adcom.com>, Ilkka Naulapää
 <digirigawa@...il.com>, Linus Torvalds <torvalds@...ux-foundation.org>,
 Al Viro <viro@...iv.linux.org.uk>, linux-trace-kernel@...r.kernel.org,
 linux-kernel@...r.kernel.org, regressions@...mhuis.info,
 Dan Carpenter <dan.carpenter@...aro.org>
Subject: 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 <minipli@...ecurity.net> 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@goodmis.org/

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