| Message ID | 20210701083842.580466-1-elver@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] perf: Require CAP_KILL if sigtrap is requested | expand |
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; > }
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
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; }
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 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(-)