Messages in this thread Patch in this message | | | From | Ondrej Mosnacek <> | Date | Thu, 1 Jul 2021 09:56:24 +0200 | Subject | Re: [PATCH] perf: Require CAP_KILL if sigtrap is requested |
| |
On Wed, Jun 30, 2021 at 2:43 PM Marco Elver <elver@google.com> wrote: > On Wed, Jun 30, 2021 at 01:13PM +0200, Ondrej Mosnacek wrote: > > On Wed, Jun 30, 2021 at 11:38 AM Marco Elver <elver@google.com> wrote: > [...] > > > +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. > > That means someone having CAP_SYS_ADMIN, but not CAP_KILL, can't perform > the desired action. Is this what you'd like?
AFAIK, such user wouldn't be allowed to directly send a signal to a different process either. So I think it makes more sense to be consistent with the existing/main CAP_KILL usage rather than with the CAP_PERFMON usage (which has its own reason to have that fallback).
I'm not the authority on capabilities nor the perf subsystem, it just didn't seem quite right to me so I wanted to raise the concern. Hopefully someone wiser than me will speak up if I talk nonsense :)
> If so, I'll just remove the wrapper, and call capable(CAP_KILL) > directly. > > > > 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; > > Not so much about perfmon_capable() but about the ptrace_may_access() > check. The condition here is supposed to be: > > want CAP_PERFMON and (CAP_KILL if sigtrap) > OR > want ptrace access (which includes a check for same thread-group and uid) > > If we did what you propose, then the ptrace check is effectively ignored > if attr.sigtrap, and that's not what we want. > > There are lots of other ways of writing the same thing, but it should > also remain readable and sticking it all into the same condition is not > readable.
Ah, I see, I missed that semantic difference... So ptrace_may_access() implies that the process doesn't need CAP_KILL to send a signal to the task, that makes sense.
In that case I'm fine with this part as it is.
> > 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...) > > That might make sense. AFAIK, the locking is already in place via > exec_update_lock. Let me investigate. > > > > + } > > > + > > > /* > > > * 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? > > I'm not a perf maintainer, so I can't give you a definitive answer. > But, this would change the ABI, so I don't think it's realistic to > request this change at this point unfortunately.
Indeed... I worry it will make troubleshooting SELinux/capability errors more confusing, but I agree it would be a potentially risky change to fix it :/
-- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
| |