Re: [PATCH v2] perf: Require CAP_KILL if sigtrap is requested

On Thu, 1 Jul 2021 at 23:41, Eric W. Biederman ebiederm@xmission.com wrote:

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.

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