Messages in this thread Patch in this message | | | From | Ondrej Mosnacek <> | Date | Wed, 30 Jun 2021 13:13:14 +0200 | Subject | Re: [PATCH] perf: Require CAP_KILL if sigtrap is requested |
| |
On Wed, Jun 30, 2021 at 11:38 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. > > 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. > > Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events") > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Marco Elver <elver@google.com> > --- > include/linux/capability.h | 5 +++++ > kernel/events/core.c | 13 ++++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/include/linux/capability.h b/include/linux/capability.h > index 65efb74c3585..1c6be4743dbe 100644 > --- a/include/linux/capability.h > +++ b/include/linux/capability.h > @@ -264,6 +264,11 @@ static inline bool bpf_capable(void) > return capable(CAP_BPF) || capable(CAP_SYS_ADMIN); > } > > +static inline bool kill_capable(void) > +{ > + return capable(CAP_KILL) || capable(CAP_SYS_ADMIN);
Is it really necessary to fall back to CAP_SYS_ADMIN here? CAP_PERFMON and CAP_BPF have been split off from CAP_SYS_ADMIN recently, so they have it for backwards compatibility. You are adding a new restriction for a very specific action, so I don't think the fallback is needed.
> +} > + > static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns) > { > return ns_capable(ns, CAP_CHECKPOINT_RESTORE) || > diff --git a/kernel/events/core.c b/kernel/events/core.c > index fe88d6eea3c2..1ab4bc867531 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -12152,10 +12152,21 @@ 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. > + */ > + is_capable &= kill_capable();
Is it necessary to do all this dance just to call perfmon_capable() first? Couldn't this be simply:
err = -EPERM; if (attr.sigtrap && !capable(CAP_KILL)) goto err_cred;
Also, looking at kill_ok_by_cred() in kernel/signal.c, would it perhaps be more appropriate to do ns_capable(__task_cred(task)->user_ns, CAP_KILL) instead? (There might also need to be some careful locking around getting the target task's creds - I'm not sure...)
> + } > + > /* > * Preserve ptrace permission check for backwards compatibility. > * > @@ -12165,7 +12176,7 @@ SYSCALL_DEFINE5(perf_event_open, > * perf_event_exit_task() that could imply). > */ > err = -EACCES;
BTW, shouldn't this (and several other such cases in this file...) actually be EPERM, as is the norm for capability checks?
> - 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; > } > > -- > 2.32.0.93.g670b81a890-goog >
-- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
| |