linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested
@ 2021-07-05  8:44 Marco Elver
  2021-07-05  8:44 ` [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() Marco Elver
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Marco Elver @ 2021-07-05  8:44 UTC (permalink / raw)
  To: elver, peterz
  Cc: tglx, mingo, dvyukov, glider, kasan-dev, linux-kernel, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
	linux-perf-users, ebiederm, omosnace, serge,
	linux-security-module, stable

If perf_event_open() is called with another task as target and
perf_event_attr::sigtrap is set, and the target task's user does not
match the calling user, also require the CAP_KILL capability or
PTRACE_MODE_ATTACH permissions.

Otherwise, with the CAP_PERFMON capability alone it would be possible
for a user to send SIGTRAP signals via perf events to another user's
tasks. This could potentially result in those tasks being terminated if
they cannot handle SIGTRAP signals.

Note: The check complements the existing capability check, but is not
supposed to supersede the ptrace_may_access() check. At a high level we
now have:

	capable of CAP_PERFMON and (CAP_KILL if sigtrap)
		OR
	ptrace_may_access(...) // also checks for same thread-group and uid

Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
Cc: <stable@vger.kernel.org> # 5.13+
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
v3:
* Upgrade ptrace mode check to ATTACH if attr.sigtrap, otherwise it's
  possible to change the target task (send signal) even if only read
  ptrace permissions were granted (reported by Eric W. Biederman).

v2: https://lkml.kernel.org/r/20210701083842.580466-1-elver@google.com
* Drop kill_capable() and just check CAP_KILL (reported by Ondrej Mosnacek).
* Use ns_capable(__task_cred(task)->user_ns, CAP_KILL) to check for
  capability in target task's ns (reported by Ondrej Mosnacek).

v1: https://lkml.kernel.org/r/20210630093709.3612997-1-elver@google.com
---
 kernel/events/core.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index fe88d6eea3c2..f79ee82e644a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12152,10 +12152,33 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (task) {
+		unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
+		bool is_capable;
+
 		err = down_read_interruptible(&task->signal->exec_update_lock);
 		if (err)
 			goto err_file;
 
+		is_capable = perfmon_capable();
+		if (attr.sigtrap) {
+			/*
+			 * perf_event_attr::sigtrap sends signals to the other
+			 * task. Require the current task to also have
+			 * CAP_KILL.
+			 */
+			rcu_read_lock();
+			is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+			rcu_read_unlock();
+
+			/*
+			 * If the required capabilities aren't available, checks
+			 * for ptrace permissions: upgrade to ATTACH, since
+			 * sending signals can effectively change the target
+			 * task.
+			 */
+			ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
+		}
+
 		/*
 		 * Preserve ptrace permission check for backwards compatibility.
 		 *
@@ -12165,7 +12188,7 @@ SYSCALL_DEFINE5(perf_event_open,
 		 * perf_event_exit_task() that could imply).
 		 */
 		err = -EACCES;
-		if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
+		if (!is_capable && !ptrace_may_access(task, ptrace_mode))
 			goto err_cred;
 	}
 
-- 
2.32.0.93.g670b81a890-goog


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

* [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission()
  2021-07-05  8:44 [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Marco Elver
@ 2021-07-05  8:44 ` Marco Elver
  2021-07-06  6:16   ` Dmitry Vyukov
  2021-07-27 13:58   ` [tip: perf/urgent] " tip-bot2 for Marco Elver
  2021-07-06  6:16 ` [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Dmitry Vyukov
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 8+ messages in thread
From: Marco Elver @ 2021-07-05  8:44 UTC (permalink / raw)
  To: elver, peterz
  Cc: tglx, mingo, dvyukov, glider, kasan-dev, linux-kernel, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
	linux-perf-users, ebiederm, omosnace, serge,
	linux-security-module

Refactor the permission check in perf_event_open() into a helper
perf_check_permission(). This makes the permission check logic more
readable (because we no longer have a negated disjunction). Add a
comment mentioning the ptrace check also checks the uid.

No functional change intended.

Signed-off-by: Marco Elver <elver@google.com>
---
v3:
* Introduce this patch to refactor the permissions checking logic to
  make it more readable (reported by Eric W. Biederman).
---
 kernel/events/core.c | 58 ++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f79ee82e644a..3008b986994b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11912,6 +11912,37 @@ __perf_event_ctx_lock_double(struct perf_event *group_leader,
 	return gctx;
 }
 
+static bool
+perf_check_permission(struct perf_event_attr *attr, struct task_struct *task)
+{
+	unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
+	bool is_capable = perfmon_capable();
+
+	if (attr->sigtrap) {
+		/*
+		 * perf_event_attr::sigtrap sends signals to the other task.
+		 * Require the current task to also have CAP_KILL.
+		 */
+		rcu_read_lock();
+		is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+		rcu_read_unlock();
+
+		/*
+		 * If the required capabilities aren't available, checks for
+		 * ptrace permissions: upgrade to ATTACH, since sending signals
+		 * can effectively change the target task.
+		 */
+		ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
+	}
+
+	/*
+	 * Preserve ptrace permission check for backwards compatibility. The
+	 * ptrace check also includes checks that the current task and other
+	 * task have matching uids, and is therefore not done here explicitly.
+	 */
+	return is_capable || ptrace_may_access(task, ptrace_mode);
+}
+
 /**
  * sys_perf_event_open - open a performance event, associate it to a task/cpu
  *
@@ -12152,43 +12183,18 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (task) {
-		unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
-		bool is_capable;
-
 		err = down_read_interruptible(&task->signal->exec_update_lock);
 		if (err)
 			goto err_file;
 
-		is_capable = perfmon_capable();
-		if (attr.sigtrap) {
-			/*
-			 * perf_event_attr::sigtrap sends signals to the other
-			 * task. Require the current task to also have
-			 * CAP_KILL.
-			 */
-			rcu_read_lock();
-			is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
-			rcu_read_unlock();
-
-			/*
-			 * If the required capabilities aren't available, checks
-			 * for ptrace permissions: upgrade to ATTACH, since
-			 * sending signals can effectively change the target
-			 * task.
-			 */
-			ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
-		}
-
 		/*
-		 * Preserve ptrace permission check for backwards compatibility.
-		 *
 		 * We must hold exec_update_lock across this and any potential
 		 * perf_install_in_context() call for this new event to
 		 * serialize against exec() altering our credentials (and the
 		 * perf_event_exit_task() that could imply).
 		 */
 		err = -EACCES;
-		if (!is_capable && !ptrace_may_access(task, ptrace_mode))
+		if (!perf_check_permission(&attr, task))
 			goto err_cred;
 	}
 
-- 
2.32.0.93.g670b81a890-goog


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

* Re: [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested
  2021-07-05  8:44 [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Marco Elver
  2021-07-05  8:44 ` [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() Marco Elver
@ 2021-07-06  6:16 ` Dmitry Vyukov
  2021-07-12 10:32 ` Marco Elver
  2021-07-27 13:58 ` [tip: perf/urgent] " tip-bot2 for Marco Elver
  3 siblings, 0 replies; 8+ messages in thread
From: Dmitry Vyukov @ 2021-07-06  6:16 UTC (permalink / raw)
  To: Marco Elver
  Cc: peterz, tglx, mingo, glider, kasan-dev, linux-kernel, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
	linux-perf-users, ebiederm, omosnace, serge,
	linux-security-module, stable

On Mon, Jul 5, 2021 at 10:45 AM Marco Elver <elver@google.com> wrote:
>
> If perf_event_open() is called with another task as target and
> perf_event_attr::sigtrap is set, and the target task's user does not
> match the calling user, also require the CAP_KILL capability or
> PTRACE_MODE_ATTACH permissions.
>
> Otherwise, with the CAP_PERFMON capability alone it would be possible
> for a user to send SIGTRAP signals via perf events to another user's
> tasks. This could potentially result in those tasks being terminated if
> they cannot handle SIGTRAP signals.
>
> Note: The check complements the existing capability check, but is not
> supposed to supersede the ptrace_may_access() check. At a high level we
> now have:
>
>         capable of CAP_PERFMON and (CAP_KILL if sigtrap)
>                 OR
>         ptrace_may_access(...) // also checks for same thread-group and uid
>
> Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
> Cc: <stable@vger.kernel.org> # 5.13+
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Marco Elver <elver@google.com>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
> v3:
> * Upgrade ptrace mode check to ATTACH if attr.sigtrap, otherwise it's
>   possible to change the target task (send signal) even if only read
>   ptrace permissions were granted (reported by Eric W. Biederman).
>
> v2: https://lkml.kernel.org/r/20210701083842.580466-1-elver@google.com
> * Drop kill_capable() and just check CAP_KILL (reported by Ondrej Mosnacek).
> * Use ns_capable(__task_cred(task)->user_ns, CAP_KILL) to check for
>   capability in target task's ns (reported by Ondrej Mosnacek).
>
> v1: https://lkml.kernel.org/r/20210630093709.3612997-1-elver@google.com
> ---
>  kernel/events/core.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fe88d6eea3c2..f79ee82e644a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12152,10 +12152,33 @@ SYSCALL_DEFINE5(perf_event_open,
>         }
>
>         if (task) {
> +               unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
> +               bool is_capable;
> +
>                 err = down_read_interruptible(&task->signal->exec_update_lock);
>                 if (err)
>                         goto err_file;
>
> +               is_capable = perfmon_capable();
> +               if (attr.sigtrap) {
> +                       /*
> +                        * perf_event_attr::sigtrap sends signals to the other
> +                        * task. Require the current task to also have
> +                        * CAP_KILL.
> +                        */
> +                       rcu_read_lock();
> +                       is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
> +                       rcu_read_unlock();
> +
> +                       /*
> +                        * If the required capabilities aren't available, checks
> +                        * for ptrace permissions: upgrade to ATTACH, since
> +                        * sending signals can effectively change the target
> +                        * task.
> +                        */
> +                       ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
> +               }
> +
>                 /*
>                  * Preserve ptrace permission check for backwards compatibility.
>                  *
> @@ -12165,7 +12188,7 @@ SYSCALL_DEFINE5(perf_event_open,
>                  * perf_event_exit_task() that could imply).
>                  */
>                 err = -EACCES;
> -               if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> +               if (!is_capable && !ptrace_may_access(task, ptrace_mode))
>                         goto err_cred;
>         }
>
> --
> 2.32.0.93.g670b81a890-goog
>

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

* Re: [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission()
  2021-07-05  8:44 ` [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() Marco Elver
@ 2021-07-06  6:16   ` Dmitry Vyukov
  2021-07-27 13:58   ` [tip: perf/urgent] " tip-bot2 for Marco Elver
  1 sibling, 0 replies; 8+ messages in thread
From: Dmitry Vyukov @ 2021-07-06  6:16 UTC (permalink / raw)
  To: Marco Elver
  Cc: peterz, tglx, mingo, glider, kasan-dev, linux-kernel, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
	linux-perf-users, ebiederm, omosnace, serge,
	linux-security-module

On Mon, Jul 5, 2021 at 10:45 AM Marco Elver <elver@google.com> wrote:
>
> Refactor the permission check in perf_event_open() into a helper
> perf_check_permission(). This makes the permission check logic more
> readable (because we no longer have a negated disjunction). Add a
> comment mentioning the ptrace check also checks the uid.
>
> No functional change intended.
>
> Signed-off-by: Marco Elver <elver@google.com>

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

> ---
> v3:
> * Introduce this patch to refactor the permissions checking logic to
>   make it more readable (reported by Eric W. Biederman).
> ---
>  kernel/events/core.c | 58 ++++++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f79ee82e644a..3008b986994b 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11912,6 +11912,37 @@ __perf_event_ctx_lock_double(struct perf_event *group_leader,
>         return gctx;
>  }
>
> +static bool
> +perf_check_permission(struct perf_event_attr *attr, struct task_struct *task)
> +{
> +       unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
> +       bool is_capable = perfmon_capable();
> +
> +       if (attr->sigtrap) {
> +               /*
> +                * perf_event_attr::sigtrap sends signals to the other task.
> +                * Require the current task to also have CAP_KILL.
> +                */
> +               rcu_read_lock();
> +               is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
> +               rcu_read_unlock();
> +
> +               /*
> +                * If the required capabilities aren't available, checks for
> +                * ptrace permissions: upgrade to ATTACH, since sending signals
> +                * can effectively change the target task.
> +                */
> +               ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
> +       }
> +
> +       /*
> +        * Preserve ptrace permission check for backwards compatibility. The
> +        * ptrace check also includes checks that the current task and other
> +        * task have matching uids, and is therefore not done here explicitly.
> +        */
> +       return is_capable || ptrace_may_access(task, ptrace_mode);
> +}
> +
>  /**
>   * sys_perf_event_open - open a performance event, associate it to a task/cpu
>   *
> @@ -12152,43 +12183,18 @@ SYSCALL_DEFINE5(perf_event_open,
>         }
>
>         if (task) {
> -               unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
> -               bool is_capable;
> -
>                 err = down_read_interruptible(&task->signal->exec_update_lock);
>                 if (err)
>                         goto err_file;
>
> -               is_capable = perfmon_capable();
> -               if (attr.sigtrap) {
> -                       /*
> -                        * perf_event_attr::sigtrap sends signals to the other
> -                        * task. Require the current task to also have
> -                        * CAP_KILL.
> -                        */
> -                       rcu_read_lock();
> -                       is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
> -                       rcu_read_unlock();
> -
> -                       /*
> -                        * If the required capabilities aren't available, checks
> -                        * for ptrace permissions: upgrade to ATTACH, since
> -                        * sending signals can effectively change the target
> -                        * task.
> -                        */
> -                       ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
> -               }
> -
>                 /*
> -                * Preserve ptrace permission check for backwards compatibility.
> -                *
>                  * We must hold exec_update_lock across this and any potential
>                  * perf_install_in_context() call for this new event to
>                  * serialize against exec() altering our credentials (and the
>                  * perf_event_exit_task() that could imply).
>                  */
>                 err = -EACCES;
> -               if (!is_capable && !ptrace_may_access(task, ptrace_mode))
> +               if (!perf_check_permission(&attr, task))
>                         goto err_cred;
>         }
>
> --
> 2.32.0.93.g670b81a890-goog
>

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

* Re: [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested
  2021-07-05  8:44 [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Marco Elver
  2021-07-05  8:44 ` [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() Marco Elver
  2021-07-06  6:16 ` [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Dmitry Vyukov
@ 2021-07-12 10:32 ` Marco Elver
  2021-07-13  9:48   ` Peter Zijlstra
  2021-07-27 13:58 ` [tip: perf/urgent] " tip-bot2 for Marco Elver
  3 siblings, 1 reply; 8+ messages in thread
From: Marco Elver @ 2021-07-12 10:32 UTC (permalink / raw)
  To: elver, peterz
  Cc: tglx, mingo, dvyukov, glider, kasan-dev, linux-kernel, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
	linux-perf-users, ebiederm, omosnace, serge,
	linux-security-module, stable

It'd be good to get this sorted -- please take another look.

Many thanks,
-- Marco

On Mon, 5 Jul 2021 at 10:45, Marco Elver <elver@google.com> wrote:
> If perf_event_open() is called with another task as target and
> perf_event_attr::sigtrap is set, and the target task's user does not
> match the calling user, also require the CAP_KILL capability or
> PTRACE_MODE_ATTACH permissions.
>
> Otherwise, with the CAP_PERFMON capability alone it would be possible
> for a user to send SIGTRAP signals via perf events to another user's
> tasks. This could potentially result in those tasks being terminated if
> they cannot handle SIGTRAP signals.
>
> Note: The check complements the existing capability check, but is not
> supposed to supersede the ptrace_may_access() check. At a high level we
> now have:
>
>         capable of CAP_PERFMON and (CAP_KILL if sigtrap)
>                 OR
>         ptrace_may_access(...) // also checks for same thread-group and uid
>
> Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
> Cc: <stable@vger.kernel.org> # 5.13+
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> v3:
> * Upgrade ptrace mode check to ATTACH if attr.sigtrap, otherwise it's
>   possible to change the target task (send signal) even if only read
>   ptrace permissions were granted (reported by Eric W. Biederman).
>
> v2: https://lkml.kernel.org/r/20210701083842.580466-1-elver@google.com
> * Drop kill_capable() and just check CAP_KILL (reported by Ondrej Mosnacek).
> * Use ns_capable(__task_cred(task)->user_ns, CAP_KILL) to check for
>   capability in target task's ns (reported by Ondrej Mosnacek).
>
> v1: https://lkml.kernel.org/r/20210630093709.3612997-1-elver@google.com
> ---
>  kernel/events/core.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fe88d6eea3c2..f79ee82e644a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12152,10 +12152,33 @@ SYSCALL_DEFINE5(perf_event_open,
>         }
>
>         if (task) {
> +               unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
> +               bool is_capable;
> +
>                 err = down_read_interruptible(&task->signal->exec_update_lock);
>                 if (err)
>                         goto err_file;
>
> +               is_capable = perfmon_capable();
> +               if (attr.sigtrap) {
> +                       /*
> +                        * perf_event_attr::sigtrap sends signals to the other
> +                        * task. Require the current task to also have
> +                        * CAP_KILL.
> +                        */
> +                       rcu_read_lock();
> +                       is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
> +                       rcu_read_unlock();
> +
> +                       /*
> +                        * If the required capabilities aren't available, checks
> +                        * for ptrace permissions: upgrade to ATTACH, since
> +                        * sending signals can effectively change the target
> +                        * task.
> +                        */
> +                       ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
> +               }
> +
>                 /*
>                  * Preserve ptrace permission check for backwards compatibility.
>                  *
> @@ -12165,7 +12188,7 @@ SYSCALL_DEFINE5(perf_event_open,
>                  * perf_event_exit_task() that could imply).
>                  */
>                 err = -EACCES;
> -               if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> +               if (!is_capable && !ptrace_may_access(task, ptrace_mode))
>                         goto err_cred;
>         }
>
> --
> 2.32.0.93.g670b81a890-goog
>

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

* Re: [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested
  2021-07-12 10:32 ` Marco Elver
@ 2021-07-13  9:48   ` Peter Zijlstra
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Zijlstra @ 2021-07-13  9:48 UTC (permalink / raw)
  To: Marco Elver
  Cc: tglx, mingo, dvyukov, glider, kasan-dev, linux-kernel, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
	linux-perf-users, ebiederm, omosnace, serge,
	linux-security-module, stable

On Mon, Jul 12, 2021 at 12:32:33PM +0200, Marco Elver wrote:
> It'd be good to get this sorted -- please take another look.

Thanks!

I'll queue them into perf/urgent.

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

* [tip: perf/urgent] perf: Refactor permissions check into perf_check_permission()
  2021-07-05  8:44 ` [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() Marco Elver
  2021-07-06  6:16   ` Dmitry Vyukov
@ 2021-07-27 13:58   ` tip-bot2 for Marco Elver
  1 sibling, 0 replies; 8+ messages in thread
From: tip-bot2 for Marco Elver @ 2021-07-27 13:58 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Marco Elver, Peter Zijlstra (Intel), Dmitry Vyukov, x86, linux-kernel

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     b068fc04de10fff8974f6ef32b861ad134d94ba4
Gitweb:        https://git.kernel.org/tip/b068fc04de10fff8974f6ef32b861ad134d94ba4
Author:        Marco Elver <elver@google.com>
AuthorDate:    Mon, 05 Jul 2021 10:44:53 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 16 Jul 2021 18:46:38 +02:00

perf: Refactor permissions check into perf_check_permission()

Refactor the permission check in perf_event_open() into a helper
perf_check_permission(). This makes the permission check logic more
readable (because we no longer have a negated disjunction). Add a
comment mentioning the ptrace check also checks the uid.

No functional change intended.

Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Link: https://lore.kernel.org/r/20210705084453.2151729-2-elver@google.com
---
 kernel/events/core.c | 58 +++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index c13730b..1cb1f9b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11917,6 +11917,37 @@ again:
 	return gctx;
 }
 
+static bool
+perf_check_permission(struct perf_event_attr *attr, struct task_struct *task)
+{
+	unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
+	bool is_capable = perfmon_capable();
+
+	if (attr->sigtrap) {
+		/*
+		 * perf_event_attr::sigtrap sends signals to the other task.
+		 * Require the current task to also have CAP_KILL.
+		 */
+		rcu_read_lock();
+		is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+		rcu_read_unlock();
+
+		/*
+		 * If the required capabilities aren't available, checks for
+		 * ptrace permissions: upgrade to ATTACH, since sending signals
+		 * can effectively change the target task.
+		 */
+		ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
+	}
+
+	/*
+	 * Preserve ptrace permission check for backwards compatibility. The
+	 * ptrace check also includes checks that the current task and other
+	 * task have matching uids, and is therefore not done here explicitly.
+	 */
+	return is_capable || ptrace_may_access(task, ptrace_mode);
+}
+
 /**
  * sys_perf_event_open - open a performance event, associate it to a task/cpu
  *
@@ -12158,43 +12189,18 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (task) {
-		unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
-		bool is_capable;
-
 		err = down_read_interruptible(&task->signal->exec_update_lock);
 		if (err)
 			goto err_file;
 
-		is_capable = perfmon_capable();
-		if (attr.sigtrap) {
-			/*
-			 * perf_event_attr::sigtrap sends signals to the other
-			 * task. Require the current task to also have
-			 * CAP_KILL.
-			 */
-			rcu_read_lock();
-			is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
-			rcu_read_unlock();
-
-			/*
-			 * If the required capabilities aren't available, checks
-			 * for ptrace permissions: upgrade to ATTACH, since
-			 * sending signals can effectively change the target
-			 * task.
-			 */
-			ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
-		}
-
 		/*
-		 * Preserve ptrace permission check for backwards compatibility.
-		 *
 		 * We must hold exec_update_lock across this and any potential
 		 * perf_install_in_context() call for this new event to
 		 * serialize against exec() altering our credentials (and the
 		 * perf_event_exit_task() that could imply).
 		 */
 		err = -EACCES;
-		if (!is_capable && !ptrace_may_access(task, ptrace_mode))
+		if (!perf_check_permission(&attr, task))
 			goto err_cred;
 	}
 

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

* [tip: perf/urgent] perf: Fix required permissions if sigtrap is requested
  2021-07-05  8:44 [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Marco Elver
                   ` (2 preceding siblings ...)
  2021-07-12 10:32 ` Marco Elver
@ 2021-07-27 13:58 ` tip-bot2 for Marco Elver
  3 siblings, 0 replies; 8+ messages in thread
From: tip-bot2 for Marco Elver @ 2021-07-27 13:58 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Dmitry Vyukov, Marco Elver, Peter Zijlstra (Intel),
	stable, x86, linux-kernel

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     9d7a6c95f62bc335b62aaf9d50590122bd03a796
Gitweb:        https://git.kernel.org/tip/9d7a6c95f62bc335b62aaf9d50590122bd03a796
Author:        Marco Elver <elver@google.com>
AuthorDate:    Mon, 05 Jul 2021 10:44:52 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 16 Jul 2021 18:46:38 +02:00

perf: Fix required permissions if sigtrap is requested

If perf_event_open() is called with another task as target and
perf_event_attr::sigtrap is set, and the target task's user does not
match the calling user, also require the CAP_KILL capability or
PTRACE_MODE_ATTACH permissions.

Otherwise, with the CAP_PERFMON capability alone it would be possible
for a user to send SIGTRAP signals via perf events to another user's
tasks. This could potentially result in those tasks being terminated if
they cannot handle SIGTRAP signals.

Note: The check complements the existing capability check, but is not
supposed to supersede the ptrace_may_access() check. At a high level we
now have:

	capable of CAP_PERFMON and (CAP_KILL if sigtrap)
		OR
	ptrace_may_access(...) // also checks for same thread-group and uid

Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org> # 5.13+
Link: https://lore.kernel.org/r/20210705084453.2151729-1-elver@google.com
---
 kernel/events/core.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4649170..c13730b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12158,10 +12158,33 @@ SYSCALL_DEFINE5(perf_event_open,
 	}
 
 	if (task) {
+		unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
+		bool is_capable;
+
 		err = down_read_interruptible(&task->signal->exec_update_lock);
 		if (err)
 			goto err_file;
 
+		is_capable = perfmon_capable();
+		if (attr.sigtrap) {
+			/*
+			 * perf_event_attr::sigtrap sends signals to the other
+			 * task. Require the current task to also have
+			 * CAP_KILL.
+			 */
+			rcu_read_lock();
+			is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+			rcu_read_unlock();
+
+			/*
+			 * If the required capabilities aren't available, checks
+			 * for ptrace permissions: upgrade to ATTACH, since
+			 * sending signals can effectively change the target
+			 * task.
+			 */
+			ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
+		}
+
 		/*
 		 * Preserve ptrace permission check for backwards compatibility.
 		 *
@@ -12171,7 +12194,7 @@ SYSCALL_DEFINE5(perf_event_open,
 		 * perf_event_exit_task() that could imply).
 		 */
 		err = -EACCES;
-		if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
+		if (!is_capable && !ptrace_may_access(task, ptrace_mode))
 			goto err_cred;
 	}
 

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

end of thread, other threads:[~2021-07-27 13:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05  8:44 [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Marco Elver
2021-07-05  8:44 ` [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() Marco Elver
2021-07-06  6:16   ` Dmitry Vyukov
2021-07-27 13:58   ` [tip: perf/urgent] " tip-bot2 for Marco Elver
2021-07-06  6:16 ` [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested Dmitry Vyukov
2021-07-12 10:32 ` Marco Elver
2021-07-13  9:48   ` Peter Zijlstra
2021-07-27 13:58 ` [tip: perf/urgent] " tip-bot2 for Marco Elver

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).