Re: [PATCH] perf: Require CAP_KILL if sigtrap is requested
From: Ondrej Mosnacek <omosnace@redhat.com>
Date: 2021-06-30 11:13:31
Also in:
linux-security-module, lkml
On Wed, Jun 30, 2021 at 11:38 AM Marco Elver [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+} + 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...)
quoted hunk ↗ jump to hunk
+ } + /* * 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.