Re: [RFC PATCH] tracing/user_events: Limit showing event names to CAP_SYS_ADMIN users

[Steven Rostedt <rostedt@goodmis.org>]

[ Added Kees Cook ]

On Fri, 11 Mar 2022 17:05:09 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> On Fri, Mar 11, 2022 at 05:01:40PM -0800, Beau Belgrave wrote:
> > Show actual names only to CAP_SYS_ADMIN capable users.
> > 
> > When user_events are configured to have broader write access than
> > default, this allows seeing names of events from other containers, etc.
> > Limit who can see the actual names to prevent event squatting or
> > information leakage.
> > 
> > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> > ---
> >  kernel/trace/trace_events_user.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > index 2b5e9fdb63a0..fb9fb2071173 100644
> > --- a/kernel/trace/trace_events_user.c
> > +++ b/kernel/trace/trace_events_user.c
> > @@ -1480,6 +1480,9 @@ static int user_seq_show(struct seq_file *m, void *p)
> >  	struct user_event *user;
> >  	char status;
> >  	int i, active = 0, busy = 0, flags;
> > +	bool show_names;
> > +
> > +	show_names = capable(CAP_SYS_ADMIN);
> >  
> >  	mutex_lock(&reg_mutex);
> >  
> > @@ -1487,7 +1490,10 @@ static int user_seq_show(struct seq_file *m, void *p)
> >  		status = register_page_data[user->index];
> >  		flags = user->flags;
> >  
> > -		seq_printf(m, "%d:%s", user->index, EVENT_NAME(user));
> > +		if (show_names)
> > +			seq_printf(m, "%d:%s", user->index, EVENT_NAME(user));
> > +		else
> > +			seq_printf(m, "%d:<hidden>", user->index);
> >  
> >  		if (flags != 0 || status != 0)
> >  			seq_puts(m, " #");
> > 
> > base-commit: 864ea0e10cc90416a01b46f0d47a6f26dc020820
> > -- 
> > 2.17.1  
> 
> I wanted to get some comments on this. I think for scenarios where
> user_events is used in a heavy cgroup environment, that we need to have
> some tracing cgroup awareness.
> 
> Has this come up before? I would like to only show user_events that have
> been created in the current cgroup (and below) like perf_events do for
> capturing.

I added Kees because he had issues with capabilities in the past wrt
tracing. Talking with him was one of the reasons I decided to push the
file permissions for who has what access.

> 
> I would also like to get to a point where we can limit how many events
> each cgroup can register under user_events.
> 
> To me, this sounds like a large feature that requires some alignment for
> getting tracing cgroup aware.

At the moment I do not have a use case in mind to evaluate the
situation. But understanding more about how this will be used by a
broader audience would be useful.

-- Steve