From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S939163AbdAEXOk (ORCPT ); Thu, 5 Jan 2017 18:14:40 -0500 Received: from mail-pf0-f178.google.com ([209.85.192.178]:33278 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935843AbdAEXOc (ORCPT ); Thu, 5 Jan 2017 18:14:32 -0500 Date: Thu, 5 Jan 2017 15:14:29 -0800 From: Kees Cook To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , John Dias , Min Chong Subject: [PATCH] perf: protect group_leader from races that cause ctx Message-ID: <20170105231429.GA83592@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: John Dias 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 Signed-off-by: Kees Cook --- 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