[for-3.18.y] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race

Message ID 1515505362-26760-1-git-send-email-amit.pundir@linaro.org
State New
Headers show
Series
  • [for-3.18.y] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race
Related show

Commit Message

Amit Pundir Jan. 9, 2018, 1:42 p.m.
From: Peter Zijlstra <peterz@infradead.org>


commit 321027c1fe77f892f4ea07846aeae08cefbbb290 upstream.

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>

[bwh: Backported to 3.16:
 - Use ACCESS_ONCE() instead of READ_ONCE()
 - Test perf_event::group_flags instead of group_caps
 - Add the err_locked cleanup block, which we didn't need before
 - Adjust context]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

---
This upstream patch is featured in recent Android Security bulletin.
Picked up this backported patch from android-3.18. Build tested on 3.18.91

 kernel/events/core.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 4 deletions(-)

-- 
2.7.4

Comments

Greg KH Jan. 10, 2018, 12:15 p.m. | #1
On Tue, Jan 09, 2018 at 07:12:42PM +0530, Amit Pundir wrote:
> From: Peter Zijlstra <peterz@infradead.org>

> 

> commit 321027c1fe77f892f4ea07846aeae08cefbbb290 upstream.

> 

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

> [bwh: Backported to 3.16:

>  - Use ACCESS_ONCE() instead of READ_ONCE()

>  - Test perf_event::group_flags instead of group_caps

>  - Add the err_locked cleanup block, which we didn't need before

>  - Adjust context]

> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

> ---

> This upstream patch is featured in recent Android Security bulletin.

> Picked up this backported patch from android-3.18. Build tested on 3.18.91


Thanks for this, now queued up.

greg k-h

Patch

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9b12efcefdf7..de3303aab7d6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7414,6 +7414,37 @@  static void mutex_lock_double(struct mutex *a, struct mutex *b)
 	mutex_lock_nested(b, SINGLE_DEPTH_NESTING);
 }
 
+/*
+ * 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 = ACCESS_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
  *
@@ -7626,14 +7657,31 @@  SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (move_group) {
-		gctx = group_leader->ctx;
+		gctx = __perf_event_ctx_lock_double(group_leader, ctx);
+
+		/*
+		 * Check if we raced against another sys_perf_event_open() call
+		 * moving the software group underneath us.
+		 */
+		if (!(group_leader->group_flags & PERF_GROUP_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;
+			}
+		}
 
 		/*
 		 * See perf_event_ctx_lock() for comments on the details
 		 * of swizzling perf_event::ctx.
 		 */
-		mutex_lock_double(&gctx->mutex, &ctx->mutex);
-
 		perf_remove_from_context(group_leader, false);
 
 		/*
@@ -7674,7 +7722,7 @@  SYSCALL_DEFINE5(perf_event_open,
 	perf_unpin_context(ctx);
 
 	if (move_group) {
-		mutex_unlock(&gctx->mutex);
+		perf_event_ctx_unlock(group_leader, gctx);
 		put_ctx(gctx);
 	}
 	mutex_unlock(&ctx->mutex);
@@ -7703,6 +7751,11 @@  SYSCALL_DEFINE5(perf_event_open,
 	fd_install(event_fd, event_file);
 	return event_fd;
 
+err_locked:
+	if (move_group)
+		perf_event_ctx_unlock(group_leader, gctx);
+	mutex_unlock(&ctx->mutex);
+	fput(event_file);
 err_context:
 	perf_unpin_context(ctx);
 	put_ctx(ctx);