Re: [PATCH RFC] perf_event: Add support for LSM and SELinux checks

[Joel Fernandes]

On Thu, Oct 10, 2019 at 07:09:49PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 10, 2019 at 11:13:33AM -0400, Joel Fernandes wrote:
> > On Thu, Oct 10, 2019 at 10:12:51AM +0200, Peter Zijlstra wrote:
> > > +static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
> > >  {
> > > -	return sysctl_perf_event_paranoid > 1;
> > > +	if (sysctl_perf_event_paranoid > -1 && !capable(CAP_SYS_ADMIN))
> > > +		return -EPERM;
> > > +
> > 
> > Here the sysctl check of > -1 also is now coupled with a CAP_SYS_ADMIN check.
> > However..
> > 
> > > +	return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
> > 
> > >  }
> > >  
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> 
> > > @@ -5862,14 +5859,8 @@ static int perf_mmap(struct file *file,
> > >  	lock_limit >>= PAGE_SHIFT;
> > >  	locked = atomic64_read(&vma->vm_mm->pinned_vm) + extra;
> > >  
> > > -	if (locked > lock_limit) {
> > > -		if (perf_paranoid_tracepoint_raw() && !capable(CAP_IPC_LOCK)) {
> > > -			ret = -EPERM;
> > > -			goto unlock;
> > > -		}
> > > -
> > > -		ret = security_perf_event_open(&event->attr,
> > > -					       PERF_SECURITY_TRACEPOINT);
> > > +	if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
> > > +		ret = perf_allow_tracepoint(&event->attr);
> > 
> > In previous code, this check did not involve a check for CAP_SYS_ADMIN.
> > 
> > I am Ok with adding the CAP_SYS_ADMIN check as well which does make sense to
> > me for tracepoint access. But it is still a change in the logic so I wanted
> > to bring it up.
> > 
> > Let me know any other thoughts and then I'll post a new patch.
> 
> Yes, I did notice, I found it weird.
> 
> If you have CAP_IPC_LIMIT you should be able to bust mlock memory
> limits, so I don't see why we should further relate that to paranoid.
> 
> The way I wrote it, we also allow to bust the limit if we have disabled
> all paranoid checks. Which makes some sense I suppose.
> 
> The original commit is this:
> 
>   459ec28ab404 ("perf_counter: Allow mmap if paranoid checks are turned off")

I am thinking we can just a new function perf_is_paranoid() that has nothing
to do with the CAP_SYS_ADMIN check and doesn't have tracepoint wording:

static inline int perf_is_paranoid(void)
{
	return sysctl_perf_event_paranoid > -1;
}

And then call that from the mmap() code:
if (locked > lock_limit && perf_is_paranoid() && !capable(CAP_IPC_LOCK)) {
	return -EPERM;
}

I don't think we need to add selinux security checks here since we are
already adding security checks earlier in mmap(). This will make the code and
its intention more clear and in line with the commit 459ec28ab404 you
mentioned. Thoughts?

thanks,

 - Joel