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. 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"; > >> Looks like extending ptrace_may_access() check for perf_events with CAP_PERFMON > > You mean the following? Exactly that. > > 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
