On Thu, 1 Jul 2021 at 23:41, Eric W. Biederman [off-list ref] wrote:

Marco Elver [off-list ref] writes:

quoted

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.

I'll add more comments. It probably also makes sense to factor the
code here into its own helper.

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.

So if attr.sigtrap the checked ptrace mode needs to switch to
PTRACE_MODE_ATTACH_REALCREDS. Otherwise, it is possible to send a
signal if only read-ptrace permissions are granted.

Is my assumption here correct?

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

Thanks,
-- Marco