Re: [PATCH] fanotify: refine the validation checks on non-dir inode mask

[Jan Kara <jack@suse.cz>]

On Mon 27-06-22 20:47:19, Amir Goldstein wrote:
> Commit ceaf69f8eadc ("fanotify: do not allow setting dirent events in
> mask of non-dir") added restrictions about setting dirent events in the
> mask of a non-dir inode mark, which does not make any sense.
> 
> For backward compatibility, these restictions were added only to new
> (v5.17+) APIs.
> 
> It also does not make any sense to set the flags FAN_EVENT_ON_CHILD or
> FAN_ONDIR in the mask of a non-dir inode.  Add these flags to the
> dir-only restriction of the new APIs as well.
> 
> Move the check of the dir-only flags for new APIs into the helper
> fanotify_events_supported(), which is only called for FAN_MARK_ADD,
> because there is no need to error on an attempt to remove the dir-only
> flags from non-dir inode.
> 
> Fixes: ceaf69f8eadc ("fanotify: do not allow setting dirent events in mask of non-dir")
> Link: https://lore.kernel.org/linux-fsdevel/20220627113224.kr2725conevh53u4@quack3.lan/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks! I've taken the patch to my tree.

								Honza

> [1] https://github.com/amir73il/ltp/commits/fan_enotdir
> [2] https://github.com/amir73il/man-pages/commits/fanotify_target_fid
> 
> 
>  fs/notify/fanotify/fanotify_user.c | 34 +++++++++++++++++-------------
>  include/linux/fanotify.h           |  4 ++++
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index c2255b440df9..b08ce0d821a7 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1513,8 +1513,15 @@ static int fanotify_test_fid(struct dentry *dentry)
>  	return 0;
>  }
>  
> -static int fanotify_events_supported(struct path *path, __u64 mask)
> +static int fanotify_events_supported(struct fsnotify_group *group,
> +				     struct path *path, __u64 mask,
> +				     unsigned int flags)
>  {
> +	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
> +	/* Strict validation of events in non-dir inode mask with v5.17+ APIs */
> +	bool strict_dir_events = FAN_GROUP_FLAG(group, FAN_REPORT_TARGET_FID) ||
> +				 (mask & FAN_RENAME);
> +
>  	/*
>  	 * Some filesystems such as 'proc' acquire unusual locks when opening
>  	 * files. For them fanotify permission events have high chances of
> @@ -1526,6 +1533,16 @@ static int fanotify_events_supported(struct path *path, __u64 mask)
>  	if (mask & FANOTIFY_PERM_EVENTS &&
>  	    path->mnt->mnt_sb->s_type->fs_flags & FS_DISALLOW_NOTIFY_PERM)
>  		return -EINVAL;
> +
> +	/*
> +	 * We shouldn't have allowed setting dirent events and the directory
> +	 * flags FAN_ONDIR and FAN_EVENT_ON_CHILD in mask of non-dir inode,
> +	 * but because we always allowed it, error only when using new APIs.
> +	 */
> +	if (strict_dir_events && mark_type == FAN_MARK_INODE &&
> +	    !d_is_dir(path->dentry) && (mask & FANOTIFY_DIRONLY_EVENT_BITS))
> +		return -ENOTDIR;
> +
>  	return 0;
>  }
>  
> @@ -1672,7 +1689,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  		goto fput_and_out;
>  
>  	if (flags & FAN_MARK_ADD) {
> -		ret = fanotify_events_supported(&path, mask);
> +		ret = fanotify_events_supported(group, &path, mask, flags);
>  		if (ret)
>  			goto path_put_and_out;
>  	}
> @@ -1695,19 +1712,6 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  	else
>  		mnt = path.mnt;
>  
> -	/*
> -	 * FAN_RENAME is not allowed on non-dir (for now).
> -	 * We shouldn't have allowed setting any dirent events in mask of
> -	 * non-dir, but because we always allowed it, error only if group
> -	 * was initialized with the new flag FAN_REPORT_TARGET_FID.
> -	 */
> -	ret = -ENOTDIR;
> -	if (inode && !S_ISDIR(inode->i_mode) &&
> -	    ((mask & FAN_RENAME) ||
> -	     ((mask & FANOTIFY_DIRENT_EVENTS) &&
> -	      FAN_GROUP_FLAG(group, FAN_REPORT_TARGET_FID))))
> -		goto path_put_and_out;
> -
>  	/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount/non-dir marks */
>  	if (mnt || !S_ISDIR(inode->i_mode)) {
>  		mask &= ~FAN_EVENT_ON_CHILD;
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index edc28555814c..e517dbcf74ed 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -111,6 +111,10 @@
>  					 FANOTIFY_PERM_EVENTS | \
>  					 FAN_Q_OVERFLOW | FAN_ONDIR)
>  
> +/* Events and flags relevant only for directories */
> +#define FANOTIFY_DIRONLY_EVENT_BITS	(FANOTIFY_DIRENT_EVENTS | \
> +					 FAN_EVENT_ON_CHILD | FAN_ONDIR)
> +
>  #define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_OUTGOING_EVENTS | \
>  					 FANOTIFY_EVENT_FLAGS)
>  
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR