[Intel-gfx] [PATCH v8 00/12] Introduce CAP_PERFMON to secure system performance monitoring and observability

[Arnaldo Carvalho de Melo]

Em Mon, Jul 13, 2020 at 03:37:51PM +0300, Alexey Budankov escreveu:
> 
> On 13.07.2020 15:17, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 13, 2020 at 12:48:25PM +0300, Alexey Budankov escreveu:
> >>
> >> On 10.07.2020 20:09, Arnaldo Carvalho de Melo wrote:
> >>> Em Fri, Jul 10, 2020 at 05:30:50PM +0300, Alexey Budankov escreveu:
> >>>> On 10.07.2020 16:31, Ravi Bangoria wrote:
> >>>>>> Currently access to perf_events, i915_perf and other performance
> >>>>>> monitoring and observability subsystems of the kernel is open only for
> >>>>>> a privileged process [1] with CAP_SYS_ADMIN capability enabled in the
> >>>>>> process effective set [2].
> > 
> >>>>>> This patch set introduces CAP_PERFMON capability designed to secure
> >>>>>> system performance monitoring and observability operations so that
> >>>>>> CAP_PERFMON would assist CAP_SYS_ADMIN capability in its governing role
> >>>>>> for performance monitoring and observability subsystems of the kernel.
> > 
> >>>>> I'm seeing an issue with CAP_PERFMON when I try to record data for a
> >>>>> specific target. I don't know whether this is sort of a regression or
> >>>>> an expected behavior.
> > 
> >>>> Thanks for reporting and root causing this case. The behavior looks like
> >>>> kind of expected since currently CAP_PERFMON takes over the related part
> >>>> of CAP_SYS_ADMIN credentials only. Actually Perf security docs [1] say
> >>>> that access control is also subject to CAP_SYS_PTRACE credentials.
> > 
> >>> I think that stating that in the error message would be helpful, after
> >>> all, who reads docs? 8-)
> > 
> >> At least those who write it :D ...
> > 
> > Everybody should read it, sure :-)
> >  
> >>> I.e., this:
> >>>
> >>> $ ./perf stat ls
> >>>   Error:
> >>>   Access to performance monitoring and observability operations is limited.
> >>> $
> >>>
> >>> Could become:
> >>>
> >>> $ ./perf stat ls
> >>>   Error:
> >>>   Access to performance monitoring and observability operations is limited.
> >>>   Right now only CAP_PERFMON is granted, you may need CAP_SYS_PTRACE.
> >>> $
> >>
> >> It would better provide reference to perf security docs in the tool output.
> > 
> > So add a 3rd line:
> > 
> > $ ./perf stat ls
> >   Error:
> >   Access to performance monitoring and observability operations is limited.
> >   Right now only CAP_PERFMON is granted, you may need CAP_SYS_PTRACE.
> >   Please read the 'Perf events and tool security' document:
> >   https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html

> If it had that patch below then message change would not be required.

Sure, but the tool should continue to work and provide useful messages
when running on kernels without that change. Pointing to the document is
valid and should be done, that is an agreed point. But the tool can do
some checks, narrow down the possible causes for the error message and
provide something that in most cases will make the user make progress.

> However this two sentences in the end of whole message would still add up:
> "Please read the 'Perf events and tool security' document:
>  https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html"

We're in violent agreement here. :-)
 
> > 
> >> Looks like extending ptrace_may_access() check for perf_events with CAP_PERFMON
> > 
> > You mean the following?
> 
> Exactly that.

Sure, lets then wait for others to chime in and then you can go ahead
and submit that patch.

Peter?

- Arnaldo
 
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 856d98c36f56..a2397f724c10 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -11595,7 +11595,7 @@ SYSCALL_DEFINE5(perf_event_open,
> >  		 * perf_event_exit_task() that could imply).
> >  		 */
> >  		err = -EACCES;
> > -		if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> > +		if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> >  			goto err_cred;
> >  	}
> > 
> >> makes monitoring simpler and even more secure to use since Perf tool need
> >> not to start/stop/single-step and read/write registers and memory and so on
> >> like a debugger or strace-like tool. What do you think?
> > 
> > I tend to agree, Peter?
> >  
> >> Alexei
> >>
> >>>
> >>> - Arnaldo
> 
> Alexei

-- 

- Arnaldo