All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf: protect group_leader from races that cause ctx
@ 2017-01-05 23:14 Kees Cook
  2017-01-06  9:32 ` Peter Zijlstra
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2017-01-05 23:14 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, John Dias, Min Chong

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-01-14 12:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-05 23:14 [PATCH] perf: protect group_leader from races that cause ctx Kees Cook
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

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.