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: <20240725150517.3184e078@gandalf.local.home>
Date: Thu, 25 Jul 2024 15:05:17 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Mathias Krause <minipli@...ecurity.net>
Cc: Ajay Kaher <ajay.kaher@...adcom.com>, Masami Hiramatsu
 <mhiramat@...nel.org>, 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>, Vasavi Sirnapalli
 <vasavi.sirnapalli@...adcom.com>, Alexey Makhalov
 <alexey.makhalov@...adcom.com>, Florian Fainelli
 <florian.fainelli@...adcom.com>, Beau Belgrave <beaub@...ux.microsoft.com>
Subject: Re: tracing: user events UAF crash report

On Thu, 25 Jul 2024 20:12:33 +0200
Mathias Krause <minipli@...ecurity.net> wrote:

> 
> >   
> >> +
> >>  	if (WARN_ON_ONCE(!schedule_work(&user->put_work))) {
> >>  		/*
> >>  		 * If we fail we must wait for an admin to attempt delete or
> >> @@ -973,6 +975,11 @@ size_t copy_nofault(void *addr, size_t bytes, struct iov_iter *i)
> >>  static struct list_head *user_event_get_fields(struct trace_event_call *call)
> >>  {
> >>  	struct user_event *user = (struct user_event *)call->data;  
> 
> Dereferencing a potentially free'd object, so 'user' is now "random" data.

This is the callback function of user->call.get_fields.

That is, we have:

	user->call.get_fields = user_event_get_fields;

And the f_start() code eventually calls trace_get_fields() that has (from a
previous email in this thread).

    trace_get_fields(struct trace_event_call *event_call)
    {
	if (!event_call->class->get_fields)
        	return &event_call->class->fields;
        return event_call->class->get_fields(event_call);
    }

Where it calls the ->class->get_fields(event_call);

that calls this function. By setting:

	user->call.get_fields = NULL;

this will never get called and no random data will be accessed.

That said, I was talking with Beau, we concluded that this shouldn't be the
responsibility of the user of event call, and should be cleaned up by the
event system.

> 
> >> +	static LIST_HEAD(head);
> >> +
> >> +	/* If the user event is about to be deleted, return no fields */
> >> +	if (!user)
> >> +		return &head;
> >>  
> >>  	return &user->fields;
> >>  }  

Here's the proper fix:

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 6ef29eba90ce..3a2d2ff1625b 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -3140,8 +3140,10 @@ EXPORT_SYMBOL_GPL(trace_add_event_call);
  */
 static void __trace_remove_event_call(struct trace_event_call *call)
 {
+	lockdep_assert_held(&event_mutex);
 	event_remove(call);
 	trace_destroy_fields(call);
+	call->get_fields = NULL;
 	free_event_filter(call->filter);
 	call->filter = NULL;
 }

Can you try it out?

-- Steve