Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1552396 > unrolled thread

[PATCH] perf: protect group_leader from races that cause ctx

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf: protect group_leader from races that cause ctx Kees Cook <keescook@chromium.org> - 2017-01-06 00:20 +0100
    Re: [PATCH] perf: protect group_leader from races that cause ctx Peter Zijlstra <peterz@infradead.org> - 2017-01-06 10:40 +0100
      Re: [PATCH] perf: protect group_leader from races that cause ctx Peter Zijlstra <peterz@infradead.org> - 2017-01-06 14:20 +0100
        Re: [PATCH] perf: protect group_leader from races that cause ctx Kees Cook <keescook@chromium.org> - 2017-01-06 21:40 +0100
          Re: [PATCH] perf: protect group_leader from races that cause ctx John Dias <joaodias@google.com> - 2017-01-07 17:30 +0100
            Re: [PATCH] perf: protect group_leader from races that cause ctx Kees Cook <keescook@chromium.org> - 2017-01-10 00:20 +0100
          Re: [PATCH] perf: protect group_leader from races that cause ctx Peter Zijlstra <peterz@infradead.org> - 2017-01-10 09:50 +0100
          Re: [PATCH] perf: protect group_leader from races that cause ctx Ingo Molnar <mingo@kernel.org> - 2017-01-10 11:20 +0100
            Re: [PATCH] perf: protect group_leader from races that cause ctx Kees Cook <keescook@chromium.org> - 2017-01-11 01:00 +0100
        [tip:perf/urgent] perf/core: Fix concurrent sys_perf_event_open()  vs. 'move_group' race tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2017-01-14 13:30 +0100

#1552396 — [PATCH] perf: protect group_leader from races that cause ctx

From: John Dias <joaodias@google.com>

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: stable@vger.kernel.org
Signed-off-by: John Dias <joaodias@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/perf_event.h |  6 ++++++
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 4741ecdb9817..a3c102ec5159 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -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;
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ab15509fab8c..853284604a7b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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);
-- 
2.7.4


-- 
Kees Cook
Nexus Security

[toc] | [next] | [standalone]

#1552642

On Thu, Jan 05, 2017 at 03:14:29PM -0800, Kees Cook wrote:
> From: John Dias <joaodias@google.com>
> 
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1552773

On Fri, Jan 06, 2017 at 10:32:51AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 05, 2017 at 03:14:29PM -0800, Kees Cook wrote:
> > From: John Dias <joaodias@google.com>
> > 
> > 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.
> 
> 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 <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: John Dias <joaodias@google.com>
Cc: Min Chong <mchong@google.com>
Fixes: f63a8daa5812 ("perf: Fix event->ctx locking")
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index b47f2f24e36a..c5d62a9f2c97 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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);

[toc] | [prev] | [next] | [standalone]

#1553092

On Fri, Jan 6, 2017 at 5:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Jan 06, 2017 at 10:32:51AM +0100, Peter Zijlstra wrote:
>> On Thu, Jan 05, 2017 at 03:14:29PM -0800, Kees Cook wrote:
>> > From: John Dias <joaodias@google.com>
>> >
>> > 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.
>>
>> 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

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

> [...]
> Reported-by: Kees Cook <keescook@chromium.org>

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

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1553688

> 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

[toc] | [prev] | [next] | [standalone]

#1554788

On Sat, Jan 7, 2017 at 8:29 AM, John Dias <joaodias@google.com> wrote:
>> 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.

> 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

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1555046

On Fri, Jan 06, 2017 at 12:39:17PM -0800, Kees Cook wrote:
> > Reported-by: Kees Cook <keescook@chromium.org>
> 
> 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

That's not a valid tag, complete lack of email address etc..

[toc] | [prev] | [next] | [standalone]

#1555108

* Kees Cook <keescook@chromium.org> wrote:

> > [...]
> > Reported-by: Kees Cook <keescook@chromium.org>
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1556061

On Tue, Jan 10, 2017 at 2:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> > [...]
>> > Reported-by: Kees Cook <keescook@chromium.org>
>>
>> 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 ...

Cleanest is probably just:

Reported-by: Di Shen

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1558957 — [tip:perf/urgent] 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 <peterz@infradead.org>
AuthorDate: Wed, 11 Jan 2017 21:09:50 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
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 <joaodias@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Min Chong <mchong@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Fixes: f63a8daa5812 ("perf: Fix event->ctx locking")
Link: http://lkml.kernel.org/r/20170106131444.GZ3174@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 72ce7d6..cbc5937 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web