Skip to contentSkip to conversation

perf: protect group_leader from races that cause ctx

PATCH In review Kees Cook 5 participants 10 messages Jan 5, 2017 – Jan 14, 2017 perf events

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 perf: protect group_leader from races that cause ctx

From: John Dias <[email protected]>

When moving a group_leader perf event from a software-context to a hardware-context, there's a race in checking and updating that context. The existing locking solution doesn't work; note that it tries to grab a lock inside the group_leader's context object, which you can only get at by going through a pointer that should be protected from these races. If two threads trigger this operation simultaneously, the refcount of 'perf_event_context' will fall to zero and the object may be freed.

To avoid that problem, and to produce a simple solution, we can just use a lock per group_leader to protect all checks on the group_leader's context. The new lock is grabbed and released when no context locks are held.

CVE-2016-6787

Reported-by Di Shen (@returnsme) of KeenLab (@keen_lab), Tencent
Fixes b04243ef7006 ("perf: Complete software pmu grouping")
Cc [email protected]
Signed-off-by John Dias <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 include/linux/perf_event.h |  6 ++++++
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 21 insertions(+)
include/linux/perf_event.h +6 -0
index 4741ecdb9817..a3c102ec5159 100644
@@ -581,6 +581,12 @@ struct perf_event {
 	int				group_caps;
 
 	struct perf_event		*group_leader;
+	/*
+	 * Protect the pmu, attributes, and context of a group leader.
+	 * Note: does not protect the pointer to the group_leader.
+	 */
+	struct mutex			group_leader_mutex;
+
 	struct pmu			*pmu;
 	void				*pmu_private;
kernel/events/core.c +15 -0
index ab15509fab8c..853284604a7b 100644
@@ -9101,6 +9101,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	if (!group_leader)
 		group_leader = event;
 
+	mutex_init(&event->group_leader_mutex);
 	mutex_init(&event->child_mutex);
 	INIT_LIST_HEAD(&event->child_list);
 

@@ -9580,6 +9581,16 @@ SYSCALL_DEFINE5(perf_event_open,
 			group_leader = NULL;
 	}
 
+	/*
+	 * Take the group_leader's group_leader_mutex before observing
+	 * anything in the group leader that leads to changes in ctx,
+	 * many of which may be changing on another thread.
+	 * In particular, we want to take this lock before deciding
+	 * whether we need to move_group.
+	 */
+	if (group_leader)
+		mutex_lock(&group_leader->group_leader_mutex);
+
 	if (pid != -1 && !(flags & PERF_FLAG_PID_CGROUP)) {
 		task = find_lively_task_by_vpid(pid);
 		if (IS_ERR(task)) {

@@ -9855,6 +9866,8 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (move_group)
 		mutex_unlock(&gctx->mutex);
 	mutex_unlock(&ctx->mutex);
+	if (group_leader)
+		mutex_unlock(&group_leader->group_leader_mutex);
 
 	if (task) {
 		mutex_unlock(&task->signal->cred_guard_mutex);

@@ -9902,6 +9915,8 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (task)
 		put_task_struct(task);
 err_group_fd:
+	if (group_leader)
+		mutex_unlock(&group_leader->group_leader_mutex);
 	fdput(group);
 err_fd:
 	put_unused_fd(event_fd);
Signature · 6 lines
2.7.4


-- 
Kees Cook
Nexus Security
maintainer
#1 lore raw
Quoted 10 lines · Kees Cook

From: John Dias <[email protected]>

When moving a group_leader perf event from a software-context to a hardware-context, there's a race in checking and updating that context. The existing locking solution doesn't work; note that it tries to grab a lock inside the group_leader's context object, which you can only get at by going through a pointer that should be protected from these races. If two threads trigger this operation simultaneously, the refcount of 'perf_event_context' will fall to zero and the object may be freed.

Quoted 4 lines

To avoid that problem, and to produce a simple solution, we can just use a lock per group_leader to protect all checks on the group_leader's context. The new lock is grabbed and released when no context locks are held.

This Changelog really stinks. I'll go try and reverse engineer the thing :-(

maintainer
#2 lore raw
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Kees Cook

From: John Dias <[email protected]>

When moving a group_leader perf event from a software-context to a hardware-context, there's a race in checking and updating that context. The existing locking solution doesn't work; note that it tries to grab a lock inside the group_leader's context object, which you can only get at by going through a pointer that should be protected from these races. If two threads trigger this operation simultaneously, the refcount of 'perf_event_context' will fall to zero and the object may be freed.

Quoted 7 lines
Quoted 4 lines

To avoid that problem, and to produce a simple solution, we can just use a lock per group_leader to protect all checks on the group_leader's context. The new lock is grabbed and released when no context locks are held.

This Changelog really stinks. I'll go try and reverse engineer the thing :-(

So the fundamental problem is a race of two sys_perf_event_open() calls trying to move the same (software) group, nothing else, the rest of the text above is misdirection and side effects.

And instead of applying the existing locking rules for this exact scenario, it invents extra locking :-(

Ok so I came up with the following, compile tested only, since no reproducer and being fairly grumpy for having to spend entirely too much time reconstructing the problem.

---
Subject: perf: Fix concurrent sys_perf_event_open() move_context race

Kees reported a race between two concurrent sys_perf_event_open() calls where both try and move the same pre-existing software group into a hardware context.

The problem is exactly that of commit f63a8daa5812 ("perf: Fix event->ctx locking"), where, while we wait for a ctx->mutex acquisition, the event->ctx relation can have changed under us.

That very same commit failed to recognise sys_perf_event_context() as an external access vector to the events and thereby didn't apply the established locking rules correctly.

So while one sys_perf_event_open() call is stuck waiting on mutex_lock_double(), the other (which owns said locks) moves the group about. So by the time the former sys_perf_event_open() acquires the locks, the context we've acquired is stale (and possibly dead).

Apply the established locking rules as per perf_event_ctx_lock_nested() to the mutex_lock_double() for the move_group case. This obviously means we need to validate state after we acquire the locks.

Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc John Dias <[email protected]>
Cc Min Chong <[email protected]>
Fixes f63a8daa5812 ("perf: Fix event->ctx locking")
Reported-by Kees Cook <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/events/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 4 deletions(-)
kernel/events/core.c +54 -4
index b47f2f24e36a..c5d62a9f2c97 100644
@@ -9518,6 +9518,37 @@ static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
 	return 0;
 }
 
+/*
+ * Variation on perf_event_ctx_lock_nested(), except we take two context
+ * mutexes.
+ */
+static struct perf_event_context *
+__perf_event_ctx_lock_double(struct perf_event *group_leader,
+			     struct perf_event_context *ctx)
+{
+	struct perf_event_context *gctx;
+
+again:
+	rcu_read_lock();
+	gctx = READ_ONCE(group_leader->ctx);
+	if (!atomic_inc_not_zero(&gctx->refcount)) {
+		rcu_read_unlock();
+		goto again;
+	}
+	rcu_read_unlock();
+
+	mutex_lock_double(&gctx->mutex, &ctx->mutex);
+
+	if (group_leader->ctx != gctx) {
+		mutex_unlock(&ctx->mutex);
+		mutex_unlock(&gctx->mutex);
+		put_ctx(gctx);
+		goto again;
+	}
+
+	return gctx;
+}
+
 /**
  * sys_perf_event_open - open a performance event, associate it to a task/cpu
  *

@@ -9761,12 +9792,31 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (move_group) {
-		gctx = group_leader->ctx;
-		mutex_lock_double(&gctx->mutex, &ctx->mutex);
+		gctx = __perf_event_ctx_lock_double(group_leader, ctx);
+
 		if (gctx->task == TASK_TOMBSTONE) {
 			err = -ESRCH;
 			goto err_locked;
 		}
+
+		/*
+		 * Check if we raced against another sys_perf_event_open() call
+		 * moving the software group underneath us.
+		 */
+		if (!(group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) {
+			/*
+			 * If someone moved the group out from under us, check
+			 * if this new event wound up on the same ctx, if so
+			 * its the regular !move_group case, otherwise fail.
+			 */
+			if (gctx != ctx) {
+				err = -EINVAL;
+				goto err_locked;
+			} else {
+				perf_event_ctx_unlock(group_leader, gctx);
+				move_group = 0;
+			}
+		}
 	} else {
 		mutex_lock(&ctx->mutex);
 	}

@@ -9868,7 +9918,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	perf_unpin_context(ctx);
 
 	if (move_group)
-		mutex_unlock(&gctx->mutex);
+		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
 
 	if (task) {

@@ -9894,7 +9944,7 @@ SYSCALL_DEFINE5(perf_event_open,
 
 err_locked:
 	if (move_group)
-		mutex_unlock(&gctx->mutex);
+		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
 /* err_file: */
 	fput(event_file);
author
#3 lore raw
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Kees Cook

From: John Dias <[email protected]>

When moving a group_leader perf event from a software-context to a hardware-context, there's a race in checking and updating that context. The existing locking solution doesn't work; note that it tries to grab a lock inside the group_leader's context object, which you can only get at by going through a pointer that should be protected from these races. If two threads trigger this operation simultaneously, the refcount of 'perf_event_context' will fall to zero and the object may be freed.

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines

To avoid that problem, and to produce a simple solution, we can just use a lock per group_leader to protect all checks on the group_leader's context. The new lock is grabbed and released when no context locks are held.

This Changelog really stinks. I'll go try and reverse engineer the thing :-(

Sorry! I tried to merge John's changelog with details from the original internal bug report. I guess I failed. :P

Quoted 10 lines

So the fundamental problem is a race of two sys_perf_event_open() calls trying to move the same (software) group, nothing else, the rest of the text above is misdirection and side effects.

And instead of applying the existing locking rules for this exact scenario, it invents extra locking :-(

Ok so I came up with the following, compile tested only, since no reproducer and being fairly grumpy for having to spend entirely too much time reconstructing the problem.

John, are you able to test this solution? IIUC, you've got a reproducer handy?

Thanks for digging into this Peter!

Quoted 2 lines
[...]
Reported-by Kees Cook <[email protected]>

I was just relaying a fix. I noted the original reporter in the first patch, how they asked to be credited:

Reported-by Di Shen (@returnsme) of KeenLab (@keen_lab), Tencent

-Kees

Signature · 2 lines
Kees Cook
Nexus Security
#4 lore raw
Quoted 1 line

John, are you able to test this solution? IIUC, you've got a reproducer handy?

Yes, I tested against two reproducers. Both were fixed by this solution.

Thanks to Kees and Peter for your patience on this. John

author
#5 lore raw
Quoted 3 lines · John Dias
Quoted 1 line

John, are you able to test this solution? IIUC, you've got a reproducer handy?

Yes, I tested against two reproducers. Both were fixed by this solution.

Great! Thanks for testing this.

Quoted 1 line

Thanks to Kees and Peter for your patience on this.

Peter, can you push your patch into the tree with the adjusted Reported-by and Cc: stable (and maybe add Tested-by from John)?

Thanks again for working on it!

-Kees

Signature · 2 lines
Kees Cook
Nexus Security
#7 lore raw
In reply to Kees Cook
Quoted 7 lines · Kees Cook
Quoted 2 lines
[...]
Reported-by Kees Cook <[email protected]>

I was just relaying a fix. I noted the original reporter in the first patch, how they asked to be credited:

Reported-by Di Shen (@returnsme) of KeenLab (@keen_lab), Tencent

So while it's OK to not give an email address (unlike Signed-off-by the Reported-by line is giving credit and has no code authorship legal effect), I agree with Peter that this line is pretty weird, bordering on the ugly.

If the reporter doesn't want the email address exposed we can just skip the tag and credit the reporter in the changelog:

Di Shen reported that ...

Thanks,

Ingo

author
#8 lore raw
Quoted 11 lines · Ingo Molnar
Quoted 7 lines · Kees Cook
Quoted 2 lines
[...]
Reported-by Kees Cook <[email protected]>

I was just relaying a fix. I noted the original reporter in the first patch, how they asked to be credited:

Reported-by Di Shen (@returnsme) of KeenLab (@keen_lab), Tencent
Quoted 8 lines

So while it's OK to not give an email address (unlike Signed-off-by the Reported-by line is giving credit and has no code authorship legal effect), I agree with Peter that this line is pretty weird, bordering on the ugly.

If the reporter doesn't want the email address exposed we can just skip the tag and credit the reporter in the changelog:

Di Shen reported that ...

Cleanest is probably just:

Reported-by Di Shen

-Kees

Signature · 2 lines
Kees Cook
Nexus Security
merge bot
#9 lore raw

[tip:perf/urgent] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race

In reply to Peter Zijlstra
perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race
Commit-ID:  321027c1fe77f892f4ea07846aeae08cefbbb290
Gitweb:     http://git.kernel.org/tip/321027c1fe77f892f4ea07846aeae08cefbbb290
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 11 Jan 2017 21:09:50 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sat, 14 Jan 2017 10:56:11 +0100

perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race

Di Shen reported a race between two concurrent sys_perf_event_open() calls where both try and move the same pre-existing software group into a hardware context.

The problem is exactly that described in commit:

f63a8daa5812 ("perf: Fix event->ctx locking")

... where, while we wait for a ctx->mutex acquisition, the event->ctx relation can have changed under us.

That very same commit failed to recognise sys_perf_event_context() as an external access vector to the events and thereby didn't apply the established locking rules correctly.

So while one sys_perf_event_open() call is stuck waiting on mutex_lock_double(), the other (which owns said locks) moves the group about. So by the time the former sys_perf_event_open() acquires the locks, the context we've acquired is stale (and possibly dead).

Apply the established locking rules as per perf_event_ctx_lock_nested() to the mutex_lock_double() for the 'move_group' case. This obviously means we need to validate state after we acquire the locks.

Reported-by Di Shen (Keen Lab)
Tested-by John Dias <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kees Cook <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Min Chong <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Stephane Eranian <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Vince Weaver <[email protected]>
Fixes f63a8daa5812 ("perf: Fix event->ctx locking")
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/events/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 4 deletions(-)
kernel/events/core.c +54 -4
index 72ce7d6..cbc5937 100644
@@ -9529,6 +9529,37 @@ static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
 	return 0;
 }
 
+/*
+ * Variation on perf_event_ctx_lock_nested(), except we take two context
+ * mutexes.
+ */
+static struct perf_event_context *
+__perf_event_ctx_lock_double(struct perf_event *group_leader,
+			     struct perf_event_context *ctx)
+{
+	struct perf_event_context *gctx;
+
+again:
+	rcu_read_lock();
+	gctx = READ_ONCE(group_leader->ctx);
+	if (!atomic_inc_not_zero(&gctx->refcount)) {
+		rcu_read_unlock();
+		goto again;
+	}
+	rcu_read_unlock();
+
+	mutex_lock_double(&gctx->mutex, &ctx->mutex);
+
+	if (group_leader->ctx != gctx) {
+		mutex_unlock(&ctx->mutex);
+		mutex_unlock(&gctx->mutex);
+		put_ctx(gctx);
+		goto again;
+	}
+
+	return gctx;
+}
+
 /**
  * sys_perf_event_open - open a performance event, associate it to a task/cpu
  *

@@ -9772,12 +9803,31 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (move_group) {
-		gctx = group_leader->ctx;
-		mutex_lock_double(&gctx->mutex, &ctx->mutex);
+		gctx = __perf_event_ctx_lock_double(group_leader, ctx);
+
 		if (gctx->task == TASK_TOMBSTONE) {
 			err = -ESRCH;
 			goto err_locked;
 		}
+
+		/*
+		 * Check if we raced against another sys_perf_event_open() call
+		 * moving the software group underneath us.
+		 */
+		if (!(group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) {
+			/*
+			 * If someone moved the group out from under us, check
+			 * if this new event wound up on the same ctx, if so
+			 * its the regular !move_group case, otherwise fail.
+			 */
+			if (gctx != ctx) {
+				err = -EINVAL;
+				goto err_locked;
+			} else {
+				perf_event_ctx_unlock(group_leader, gctx);
+				move_group = 0;
+			}
+		}
 	} else {
 		mutex_lock(&ctx->mutex);
 	}

@@ -9879,7 +9929,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	perf_unpin_context(ctx);
 
 	if (move_group)
-		mutex_unlock(&gctx->mutex);
+		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
 
 	if (task) {

@@ -9905,7 +9955,7 @@ SYSCALL_DEFINE5(perf_event_open,
 
 err_locked:
 	if (move_group)
-		mutex_unlock(&gctx->mutex);
+		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
 /* err_file: */
 	fput(event_file);