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: <20240723104348.645bf027@gandalf.local.home>
Date: Tue, 23 Jul 2024 10:43:48 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Mathias Krause <minipli@...ecurity.net>
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 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?

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

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

>  		ei = NULL;
>  	}
>  	return ei;

Thanks,

-- Steve