Re: [PATCH 2/2] fanotify: Add pidfd support to the fanotify API

[Matthew Bobrowski <repnop@google.com>]

On Fri, Apr 16, 2021 at 10:53:48AM +0300, Amir Goldstein wrote:
> On Fri, Apr 16, 2021 at 10:06 AM Matthew Bobrowski <repnop@google.com> wrote:
> > > > +               pidfd = pidfd_create(event->pid, 0);
> > > > +               if (unlikely(pidfd < 0))
> > > > +                       metadata.pid = FAN_NOPIDFD;
> > > > +               else
> > > > +                       metadata.pid = pidfd;
> > > > +       } else {
> > > > +               metadata.pid = pid_vnr(event->pid);
> > > > +       }
> > >
> > > You should rebase your work on:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
> > > and resolve conflicts with "unprivileged listener" code.
> >
> > ACK.
> >
> > > Need to make sure that pidfd is not reported to an unprivileged
> > > listener even if group was initialized by a privileged process.
> > > This is a conscious conservative choice that we made for reporting
> > > pid info to unprivileged listener that can be revisited in the future.
> >
> > OK, I see. In that case, I guess I can add the FAN_REPORT_PIDFD check
> > above the current conditional [0]:
> >
> > ...
> > if (!capable(CAP_SYS_ADMIN) && task_tgid(current) != event->pid)
> >         metadata.pid = 0;
> > ...
> >
> > That way, AFAIK even if it is an unprivileged listener the pid info
> > will be overwritten as intended.
> >
> 
> Situation is a bit more subtle than that.
> If you override event->pid with zero and zero is interpreted as pidfd
> that would not be consistent with uapi documentation.

Ah, yes, of course! I had totally overlooked this. Also, speaking of
UAPI documentation, I'll have it prepared along with the LTP tests
once I get the ACK for this particular concept from Jan and Christian.

> You need to make sure that event->pid is FAN_NOPIDFD in case
> (!capable(CAP_SYS_ADMIN) &&
>  FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD))
> Hopefully, you can do that while keeping the special cases to minimum...

I don't foresee any issues with doing this at all.

/M