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

On Fri, Apr 16, 2021 at 10:53:48AM +0300, Amir Goldstein wrote:

On Fri, Apr 16, 2021 at 10:06 AM Matthew Bobrowski [off-list ref] wrote:

quoted

quoted

quoted

+               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.

quoted

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