Marco Elver elver@google.com writes:

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.

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

Is there anyway we could have a comment that makes the required capability checks clear?

Basically I see an inlined version of kill_ok_by_cred being implemented without the comments on why the various pieces make sense.

Certainly ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS) should not be a check to allow writing/changing a task. It needs to be PTRACE_MODE_ATTACH_REALCREDS, like /proc/self/mem uses.

Now in practice I think your patch probably has the proper checks in place for sending a signal but it is far from clear.

Eric

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

---

v2:

• 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).

---

kernel/events/core.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c index fe88d6eea3c2..43c99695dc3f 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -12152,10 +12152,23 @@ SYSCALL_DEFINE5(perf_event_open, } if (task) {

• 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 have CAP_KILL.
  

• 	 */
  

• 	rcu_read_lock();
  

• 	is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
  

• 	rcu_read_unlock();
  

• }
  

• /*
  • Preserve ptrace permission check for backwards compatibility.

@@ -12165,7 +12178,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_READ_REALCREDS))
  goto err_cred;
  

  }