All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	John Dias <joaodias@google.com>, Min Chong <mchong@google.com>
Subject: [PATCH] perf: protect group_leader from races that cause ctx
Date: Thu, 5 Jan 2017 15:14:29 -0800	[thread overview]
Message-ID: <20170105231429.GA83592@beast> (raw)

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

             reply	other threads:[~2017-01-05 23:14 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-05 23:14 Kees Cook [this message]
2017-01-06  9:32 ` Peter Zijlstra
2017-01-06 13:14   ` Peter Zijlstra
2017-01-06 20:39     ` Kees Cook
2017-01-07 16:29       ` John Dias
2017-01-09 23:16         ` Kees Cook
2017-01-10  8:46       ` Peter Zijlstra
2017-01-10 10:16       ` Ingo Molnar
2017-01-10 23:54         ` Kees Cook
2017-01-14 12:28     ` [tip:perf/urgent] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race tip-bot for Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170105231429.GA83592@beast \
    --to=keescook@chromium.org \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=joaodias@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mchong@google.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --subject='Re: [PATCH] perf: protect group_leader from races that cause ctx' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.