| Message ID | 20210705084453.2151729-1-elver@google.com |
|---|---|
| State | Accepted |
| Commit | 9d7a6c95f62bc335b62aaf9d50590122bd03a796 |
| Headers | show |
| Series | [v3,1/2] perf: Fix required permissions if sigtrap is requested | expand |
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 >
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 >
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.
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; }
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(-)