Re: [RFC][PATCH] fanotify: disallow mount/sb marks on kernel internal pseudo fs

[Christian Brauner <brauner@kernel.org>]

On Tue, Jul 04, 2023 at 01:18:33PM +0200, Jan Kara wrote:
> On Tue 04-07-23 11:58:07, Christian Brauner wrote:
> > On Mon, Jul 03, 2023 at 01:25:51PM +0200, Jan Kara wrote:
> > > On Sat 01-07-23 19:25:14, Amir Goldstein wrote:
> > > > On Fri, Jun 30, 2023 at 10:29 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > On Thu, Jun 29, 2023 at 07:20:44AM +0300, Amir Goldstein wrote:
> > > > > > Hopefully, nobody is trying to abuse mount/sb marks for watching all
> > > > > > anonymous pipes/inodes.
> > > > > >
> > > > > > I cannot think of a good reason to allow this - it looks like an
> > > > > > oversight that dated back to the original fanotify API.
> > > > > >
> > > > > > Link: https://lore.kernel.org/linux-fsdevel/20230628101132.kvchg544mczxv2pm@quack3/
> > > > > > Fixes: d54f4fba889b ("fanotify: add API to attach/detach super block mark")
> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > Jan,
> > > > > >
> > > > > > As discussed, allowing sb/mount mark on anonymous pipes
> > > > > > makes no sense and we should not allow it.
> > > > > >
> > > > > > I've noted FAN_MARK_FILESYSTEM as the Fixes commit as a trigger to
> > > > > > backport to maintained LTS kernels event though this dates back to day one
> > > > > > with FAN_MARK_MOUNT. Not sure if we should keep the Fixes tag or not.
> > > > > >
> > > > > > The reason this is an RFC and that I have not included also the
> > > > > > optimization patch is because we may want to consider banning kernel
> > > > > > internal inodes from fanotify and/or inotify altogether.
> > > > > >
> > > > > > The tricky point in banning anonymous pipes from inotify, which
> > > > > > could have existing users (?), but maybe not, so maybe this is
> > > > > > something that we need to try out.
> > > > > >
> > > > > > I think we can easily get away with banning anonymous pipes from
> > > > > > fanotify altogeter, but I would not like to get to into a situation
> > > > > > where new applications will be written to rely on inotify for
> > > > > > functionaly that fanotify is never going to have.
> > > > > >
> > > > > > Thoughts?
> > > > > > Am I over thinking this?
> > > > > >
> > > > > > Amir.
> > > > > >
> > > > > >  fs/notify/fanotify/fanotify_user.c | 14 ++++++++++++++
> > > > > >  1 file changed, 14 insertions(+)
> > > > > >
> > > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > > > > index 95d7d8790bc3..8240a3fdbef0 100644
> > > > > > --- a/fs/notify/fanotify/fanotify_user.c
> > > > > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > > > > @@ -1622,6 +1622,20 @@ static int fanotify_events_supported(struct fsnotify_group *group,
> > > > > >           path->mnt->mnt_sb->s_type->fs_flags & FS_DISALLOW_NOTIFY_PERM)
> > > > > >               return -EINVAL;
> > > > > >
> > > > > > +     /*
> > > > > > +      * mount and sb marks are not allowed on kernel internal pseudo fs,
> > > > > > +      * like pipe_mnt, because that would subscribe to events on all the
> > > > > > +      * anonynous pipes in the system.
> > > > >
> > > > > s/anonynous/anonymous/
> > > > >
> > > > > > +      *
> > > > > > +      * XXX: SB_NOUSER covers all of the internal pseudo fs whose objects
> > > > > > +      * are not exposed to user's mount namespace, but there are other
> > > > > > +      * SB_KERNMOUNT fs, like nsfs, debugfs, for which the value of
> > > > > > +      * allowing sb and mount mark is questionable.
> > > > > > +      */
> > > > > > +     if (mark_type != FAN_MARK_INODE &&
> > > > > > +         path->mnt->mnt_sb->s_flags & SB_NOUSER)
> > > > > > +             return -EINVAL;
> > > > >
> > > > 
> > > > On second thought, I am not sure about  the EINVAL error code here.
> > > > I used the same error code that Jan used for permission events on
> > > > proc fs, but the problem is that applications do not have a decent way
> > > > to differentiate between
> > > > "sb mark not supported by kernel" (i.e. < v4.20) vs.
> > > > "sb mark not supported by fs" (the case above)
> > > > 
> > > > same for permission events:
> > > > "kernel compiled without FANOTIFY_ACCESS_PERMISSIONS" vs.
> > > > "permission events not supported by fs" (procfs)
> > > > 
> > > > I have looked for other syscalls that react to SB_NOUSER and I've
> > > > found that mount also returns EINVAL.
> > > 
> > > We tend to return EINVAL both for invalid (combination of) flags as well as
> > > for flags applied to invalid objects in various calls. In practice there is
> > > rarely a difference.
> > > 
> > > > So far, fanotify_mark() and fanotify_init() mostly return EINVAL
> > > > for invalid flag combinations (also across the two syscalls),
> > > > but not because of the type of object being marked, except for
> > > > the special case of procfs and permission events.
> > > > 
> > > > mount(2) syscall OTOH, has many documented EINVAL cases
> > > > due to the type of source object (e.g. propagation type shared).
> > > > 
> > > > I know there is no standard and EINVAL can mean many
> > > > different things in syscalls, but I thought that maybe EACCES
> > > > would convey more accurately the message:
> > > > "The sb/mount of this fs is not accessible for placing a mark".
> > > > 
> > > > WDYT? worth changing?
> > > > worth changing procfs also?
> > > > We don't have that EINVAL for procfs documented in man page btw.
> > > 
> > > Well, EACCES translates to message "Permission denied" which as Christian
> > > writes is justifiable but frankly I find it more confusing. Because when I
> > > get "Permission denied", I go looking which permissions are wrong, perhaps
> > > suspecting SELinux or other LSM and don't think that object type / location
> > > is at fault.
> > > 
> > > I agree that with EINVAL it is impossible to distinguish "unsupported on
> > > this object only" vs "completely unknown flag" but it doesn't seem like a
> > > huge problem for userspace to me as I can think of workarounds even if
> > > userspace wants to do something else than "report error and bail".
> > 
> > Userspace is pretty used to the flood of EINVAL from the vfs apis so
> > they often have good workarounds. It doesn't mean it's something we
> > should just discount ofc. I think having ways to surface more
> > descriptive errors would overall be a good thing.
> 
> Oh, I absolutely agree with that. I'm just not sure whether returning
> EACCES in this particular case is going to cause more or less confusion.

Yeah, I really meant it as a generic comment that also applies to my
own comment about EINVAL being ubiquitous; so not directed at your
comment about EACCES specifically.