Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 30 Jun 2021 14:42:49 +0200
From:   Marco Elver <elver@google.com>
To:     Ondrej Mosnacek <omosnace@redhat.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>, kasan-dev@googlegroups.com,
        Linux kernel mailing list <linux-kernel@vger.kernel.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Linux Security Module list 
        <linux-security-module@vger.kernel.org>,
        linux-perf-users@vger.kernel.org,
        Eric Biederman <ebiederm@xmission.com>,
        Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH] perf: Require CAP_KILL if sigtrap is requested
Message-ID: <YNxmyRYcs/R/8zry@elver.google.com>
References: <20210630093709.3612997-1-elver@google.com>
 <CAFqZXNtaHyKjcOmh4_5AUfm0mek6Zx0V1TvN8BwHNK9Q7T3D8w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFqZXNtaHyKjcOmh4_5AUfm0mek6Zx0V1TvN8BwHNK9Q7T3D8w@mail.gmail.com>
User-Agent: Mutt/2.0.5 (2021-01-21)
Precedence: bulk

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?

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.

> 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.

Thanks.