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

[Christian Brauner <brauner@kernel.org>]

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;

It's a good starting point.

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>