linux-kernel - Re: possible deadlock in proc_pid

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: <20200828123720.GZ1362448@hirez.programming.kicks-ass.net>
Date:   Fri, 28 Aug 2020 14:37:20 +0200
From:   peterz@...radead.org
To:     ebiederm@...ssion.com (Eric W. Biederman)
Cc:     syzbot <syzbot+db9cdf3dd1f64252c6ef@...kaller.appspotmail.com>,
        adobriyan@...il.com, akpm@...ux-foundation.org, avagin@...il.com,
        christian@...uner.io, gladkov.alexey@...il.com,
        keescook@...omium.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        walken@...gle.com, Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Miklos Szeredi <miklos@...redi.hu>, jannh@...gle.com
Subject: Re: possible deadlock in proc_pid_syscall (2)

On Fri, Aug 28, 2020 at 07:01:17AM -0500, Eric W. Biederman wrote:
> This feels like an issue where perf can just do too much under
> exec_update_mutex.  In particular calling kern_path from
> create_local_trace_uprobe.  Calling into the vfs at the very least
> makes it impossible to know exactly which locks will be taken.
> 
> Thoughts?

> > -> #1 (&ovl_i_mutex_dir_key[depth]){++++}-{3:3}:
> >        down_read+0x96/0x420 kernel/locking/rwsem.c:1492
> >        inode_lock_shared include/linux/fs.h:789 [inline]
> >        lookup_slow fs/namei.c:1560 [inline]
> >        walk_component+0x409/0x6a0 fs/namei.c:1860
> >        lookup_last fs/namei.c:2309 [inline]
> >        path_lookupat+0x1ba/0x830 fs/namei.c:2333
> >        filename_lookup+0x19f/0x560 fs/namei.c:2366
> >        create_local_trace_uprobe+0x87/0x4e0 kernel/trace/trace_uprobe.c:1574
> >        perf_uprobe_init+0x132/0x210 kernel/trace/trace_event_perf.c:323
> >        perf_uprobe_event_init+0xff/0x1c0 kernel/events/core.c:9580
> >        perf_try_init_event+0x12a/0x560 kernel/events/core.c:10899
> >        perf_init_event kernel/events/core.c:10951 [inline]
> >        perf_event_alloc.part.0+0xdee/0x3770 kernel/events/core.c:11229
> >        perf_event_alloc kernel/events/core.c:11608 [inline]
> >        __do_sys_perf_event_open+0x72c/0x2cb0 kernel/events/core.c:11724
> >        do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> >        entry_SYSCALL_64_after_hwframe+0x44/0xa9

Right, so we hold the mutex fairly long there, supposedly to ensure
privs don't change out from under us.

We do the permission checks early -- no point in doing anything else if
we're not allowed, but we then have to hold this mutex until the event
is actually installed according to that comment.

/me goes look at git history:

  6914303824bb5 - changed cred_guard_mutex into exec_update_mutex
  79c9ce57eb2d5 - introduces cred_guard_mutex

So that latter commit explains the race we're guarding against. Without
this we can install the event after execve() which might have changed
privs on us.

I'm open to suggestions on how to do this differently.

Could we check privs twice instead?

Something like the completely untested below..

---
diff --git a/include/linux/freelist.h b/include/linux/freelist.h
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5bfe8e3c6e44..14e6c9bbfcda 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11701,21 +11701,9 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (task) {
-		err = mutex_lock_interruptible(&task->signal->exec_update_mutex);
-		if (err)
-			goto err_task;
-
-		/*
-		 * Preserve ptrace permission check for backwards compatibility.
-		 *
-		 * We must hold exec_update_mutex across this and any potential
-		 * perf_install_in_context() call for this new event to
-		 * serialize against exec() altering our credentials (and the
-		 * perf_event_exit_task() that could imply).
-		 */
 		err = -EACCES;
 		if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
-			goto err_cred;
+			goto err_task;
 	}
 
 	if (flags & PERF_FLAG_PID_CGROUP)
@@ -11844,6 +11832,24 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_context;
 	}
 
+	if (task) {
+		err = mutex_lock_interruptible(&task->signal->exec_update_mutex);
+		if (err)
+			goto err_file;
+
+		/*
+		 * Preserve ptrace permission check for backwards compatibility.
+		 *
+		 * We must hold exec_update_mutex across this and any potential
+		 * perf_install_in_context() call for this new event to
+		 * serialize against exec() altering our credentials (and the
+		 * perf_event_exit_task() that could imply).
+		 */
+		err = -EACCES;
+		if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
+			goto err_cred;
+	}
+
 	if (move_group) {
 		gctx = __perf_event_ctx_lock_double(group_leader, ctx);
 
@@ -12019,7 +12025,10 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (move_group)
 		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
-/* err_file: */
+err_cred:
+	if (task)
+		mutex_unlock(&task->signal->exec_update_mutex);
+err_file:
 	fput(event_file);
 err_context:
 	perf_unpin_context(ctx);
@@ -12031,9 +12040,6 @@ SYSCALL_DEFINE5(perf_event_open,
 	 */
 	if (!event_file)
 		free_event(event);
-err_cred:
-	if (task)
-		mutex_unlock(&task->signal->exec_update_mutex);
 err_task:
 	if (task)
 		put_task_struct(task);