Re: [PATCH] fanotify: fix permission model of unprivileged group

[Matthew Bobrowski <repnop@google.com>]

On Mon, May 24, 2021 at 12:00:04PM +0200, Jan Kara wrote:
> On Mon 24-05-21 18:41:36, Matthew Bobrowski wrote:
> > On Sat, May 22, 2021 at 12:19:16PM +0300, Amir Goldstein wrote:
> > > Reporting event->pid should depend on the privileges of the user that
> > > initialized the group, not the privileges of the user reading the
> > > events.
> > > 
> > > Use an internal group flag FANOTIFY_UNPRIV to record the fact the the
> > > group was initialized by an unprivileged user.
> > > 
> > > To be on the safe side, the premissions to setup filesystem and mount
> > > marks now require that both the user that initialized the group and
> > > the user setting up the mark have CAP_SYS_ADMIN.
> > > 
> > > Fixes: 7cea2a3c505e ("fanotify: support limited functionality for unprivileged users")
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > 
> > Thanks for sending through this patch Amir!
> > 
> > In general, the patch looks good to me, however there's just a few
> > nits below.
> > 
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index 71fefb30e015..7df6cba4a06d 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -424,11 +424,18 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
> > >  	 * events generated by the listener process itself, without disclosing
> > >  	 * the pids of other processes.
> > >  	 */
> > > -	if (!capable(CAP_SYS_ADMIN) &&
> > > +	if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
> > >  	    task_tgid(current) != event->pid)
> > >  		metadata.pid = 0;
> > >  
> > > -	if (path && path->mnt && path->dentry) {
> > > +	/*
> > > +	 * For now, we require fid mode for unprivileged listener, which does
> > > +	 * record path events, but keep this check for safety in case we want
> > > +	 * to allow unprivileged listener to get events with no fd and no fid
> > > +	 * in the future.
> > > +	 */
> > 
> > I think it's best if we keep clear of using first person in our
> > comments throughout our code base. Maybe we could change this to:
> > 
> > * For now, fid mode is required for an unprivileged listener, which
> >   does record path events. However, this check must be kept...
> 
> Actually, I have no problem with the first person in comments. It is a
> standard "anonymous" language and IMO easy to understand as well. Also
> frequently used in the kernel AFAICT. What problem do you see with the
> first person? I'm well aware that unlike us you are a native speaker ;)

That's fair, perhaps it's just personal preference more than
anything. I do believe that it does lead to more succinct comments
that doesn't necessarily lead to thinking about who the reader is
intended to be in this partcular context.

> > > +/* Internal flags */
> > > +#define FANOTIFY_UNPRIV		0x80000000
> > > +#define FANOTIFY_INTERNAL_FLAGS	(FANOTIFY_UNPRIV)
> > 
> > Should we be more distinct here i.e. FANOTIFY_INTERNAL_INIT_FLAGS?
> > Just thinking about a possible case where there's some other internal
> > fanotify flags that are used for something else?
> 
> Well, do we need to distinguish different uses of internal flags? I don't
> think so...

Maybe not. I was just thinking about the possibility of other internal
flags being possibly introduced further on down the line that wouldn't
properly align with the use of FANOTIFY_INTERNAL_FLAGS, therefore me
providing the suggestion for renaming it. Anyway, if such a situation
ever arises then there's absolutely no reason why we can't shuffle
things around later.

/M