[PATCH v2] fanotify: do not allow setting dirent events in mask of non-dir

[PATCH v2] fanotify: do not allow setting dirent events in mask of non-dir

[Amir Goldstein]

Dirent events (create/delete/move) are only reported on watched
directory inodes, but in fanotify as well as in legacy inotify, it was
always allowed to set them on non-dir inode, which does not result in
any meaningful outcome.

Until kernel v5.17, dirent events in fanotify also differed from events
"on child" (e.g. FAN_OPEN) in the information provided in the event.
For example, FAN_OPEN could be set in the mask of a non-dir or the mask
of its parent and event would report the fid of the child regardless of
the marked object.
By contrast, FAN_DELETE is not reported if the child is marked and the
child fid was not reported in the events.

Since kernel v5.17, with fanotify group flag FAN_REPORT_TARGET_FID, the
fid of the child is reported with dirent events, like events "on child",
which may create confusion for users expecting the same behavior as
events "on child" when setting events in the mask on a child.

The desired semantics of setting dirent events in the mask of a child
are not clear, so for now, deny this action for a group initialized
with flag FAN_REPORT_TARGET_FID and for the new event FAN_RENAME.
We may relax this restriction in the future if we decide on the
semantics and implement them.

Fixes: d61fd650e9d2 ("fanotify: introduce group flag FAN_REPORT_TARGET_FID")
Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Jan,

Having slept over it, I think we should apply this stronger fix.
I could have done this as an extra patch, but since FAN_RENAME was
merged together with FAN_REPORT_TARGET_FID I did not see the point,
but you could apply this on top of the FAN_RENAME patch if you prefer.

Thanks,
Amir.

Changes since v1:
- Deny all dirent events on non-dir not only FAN_RENAME
- Return -ENOTDIR instead of -EINVAL

 fs/notify/fanotify/fanotify_user.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index edad67d674dc..cf587ba2ba92 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1695,6 +1695,19 @@ 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;
-- 
2.25.1

Re: [PATCH v2] fanotify: do not allow setting dirent events in mask of non-dir

[Jan Kara]

On Sat 07-05-22 11:00:28, Amir Goldstein wrote:
> Dirent events (create/delete/move) are only reported on watched
> directory inodes, but in fanotify as well as in legacy inotify, it was
> always allowed to set them on non-dir inode, which does not result in
> any meaningful outcome.
> 
> Until kernel v5.17, dirent events in fanotify also differed from events
> "on child" (e.g. FAN_OPEN) in the information provided in the event.
> For example, FAN_OPEN could be set in the mask of a non-dir or the mask
> of its parent and event would report the fid of the child regardless of
> the marked object.
> By contrast, FAN_DELETE is not reported if the child is marked and the
> child fid was not reported in the events.
> 
> Since kernel v5.17, with fanotify group flag FAN_REPORT_TARGET_FID, the
> fid of the child is reported with dirent events, like events "on child",
> which may create confusion for users expecting the same behavior as
> events "on child" when setting events in the mask on a child.
> 
> The desired semantics of setting dirent events in the mask of a child
> are not clear, so for now, deny this action for a group initialized
> with flag FAN_REPORT_TARGET_FID and for the new event FAN_RENAME.
> We may relax this restriction in the future if we decide on the
> semantics and implement them.
> 
> Fixes: d61fd650e9d2 ("fanotify: introduce group flag FAN_REPORT_TARGET_FID")
> Fixes: 8cc3b1ccd930 ("fanotify: wire up FAN_RENAME event")
> Link: https://lore.kernel.org/linux-fsdevel/20220505133057.zm5t6vumc4xdcnsg@quack3.lan/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Jan,
> 
> Having slept over it, I think we should apply this stronger fix.
> I could have done this as an extra patch, but since FAN_RENAME was
> merged together with FAN_REPORT_TARGET_FID I did not see the point,
> but you could apply this on top of the FAN_RENAME patch if you prefer.

OK, done and force-pushed everywhere.

								Honza

> Changes since v1:
> - Deny all dirent events on non-dir not only FAN_RENAME
> - Return -ENOTDIR instead of -EINVAL
> 
>  fs/notify/fanotify/fanotify_user.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index edad67d674dc..cf587ba2ba92 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1695,6 +1695,19 @@ 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;
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR