Re: [PATCH] fanotify: report names longer than NAME_MAX

[Jan Kara]

On Sat 26-09-26 10:08:51, Matthias Goergens wrote:
> fanotify keeps the length of a reported name in a u8, so it drops any
> name longer than NAME_MAX and warns:
> 
>   WARNING: fs/notify/fanotify/fanotify.h:217 at fanotify_handle_event+0x2e82/0x39f0
> 
> Such names are legitimate on some filesystems.  vfat mounted with utf8
> takes up to 255 UTF-16 characters, which are up to 765 bytes of UTF-8,
> and FUSE accepts names up to PATH_MAX - 1.  readdir() and inotify report
> them in full.  fanotify sends the event without the name, and for
> FAN_RENAME it trips a second warning in copy_fid_info_to_user(), after
> which read() fails with EFAULT and the event is lost.
> 
> When syzbot hit this, Jan suggested accommodating names up to PATH_MAX
> [1], the limit readdir() already applies to a name
> (verify_dirent_name()).  Store the name lengths as u16, which keeps
> struct fanotify_info at 8 bytes, and drop without a warning only names
> of PATH_MAX bytes or more, which no path can refer to.  An event with
> such a name can exceed a page, so allocate name events with kvmalloc(),
> as Jan also suggested.  Names up to NAME_MAX are reported exactly as
> before, and the record format is unchanged: an event with a long name is
> just longer.
> 
> Link: https://lore.kernel.org/all/20241112115059.ecvomkalee5m4i4i@quack3/ [1]
> Fixes: 2d9374f09513 ("fanotify: use macros to get the offset to fanotify_info buffer")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>

Matthias, this patch (with slight modification for >= PATH_MAX names) is in
my tree. Would you have time to write an LTP test for fanotify which
exercises these paths longer than NAME_MAX please? So that this code gets
regular testing... Thanks!

								Honza

> ---
> 
> Tested in a VM with KASAN, lockdep and kmemleak: on vfat (utf8), names
> of 258 to 765 bytes now arrive with their name, including FAN_RENAME,
> for root and for an unprivileged watcher; FUSE names up to 4095 bytes
> likewise, also a FAN_RENAME between two of them.  With kvmalloc()'s
> kmalloc() attempt forced to fail, the events arrive intact from vmalloc.
> No warnings.  The LTP fanotify and inotify tests give the same results
> as before (fanotify20 skipped in both).
> 
> 5.15.y fails the same way (the same warnings and EFAULT on 5.15.221),
> and the patch applies there and fixes it.
> 
> Honza, on the msdos report [2] you pointed at the filesystem, and msdos
> now rejects such names (9d7ed813ee5f).  vfat cannot do the same without
> making files that other systems wrote unreachable, and FUSE allows them
> by design.
> 
> [2] https://lore.kernel.org/all/3kzvlb3wpzbg6k6ttzp6rh2cfmz3hmmmlisgxjn5u4xmaaqibm@qkre7flkhejl/
> 
>  fs/notify/fanotify/fanotify.c |  4 ++--
>  fs/notify/fanotify/fanotify.h | 20 +++++++++++++-------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index a208a7ec1692..c8289aa8a70a 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -656,7 +656,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *dir,
>  		size += FANOTIFY_FH_HDR_LEN + dir2_fh_len;
>  	if (child_fh_len)
>  		size += FANOTIFY_FH_HDR_LEN + child_fh_len;
> -	fne = kmalloc(size, gfp);
> +	fne = kvmalloc(size, gfp);
>  	if (!fne)
>  		return NULL;
>  
> @@ -1050,7 +1050,7 @@ static void fanotify_free_fid_event(struct fanotify_event *event)
>  
>  static void fanotify_free_name_event(struct fanotify_event *event)
>  {
> -	kfree(FANOTIFY_NE(event));
> +	kvfree(FANOTIFY_NE(event));
>  }
>  
>  static void fanotify_free_error_event(struct fsnotify_group *group,
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index 3710543dbf82..4aad60d2aaf5 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -43,9 +43,16 @@ struct fanotify_info {
>  	u8 dir_fh_totlen;
>  	u8 dir2_fh_totlen;
>  	u8 file_fh_totlen;
> -	u8 name_len;
> -	u8 name2_len;
> -	u8 pad[3];
> +	u8 pad;
> +	/*
> +	 * Deliberately not limited to NAME_MAX: some filesystems return longer
> +	 * names (vfat up to 255 UTF-16 characters, as Windows allows, which is
> +	 * up to 765 bytes of UTF-8; FUSE up to PATH_MAX - 1), and readdir() and
> +	 * inotify report them in full. Only names of PATH_MAX bytes or more,
> +	 * which no path can refer to, are dropped.
> +	 */
> +	u16 name_len;
> +	u16 name2_len;
>  	unsigned char buf[];
>  	/*
>  	 * (struct fanotify_fh) dir_fh starts at buf[0]
> @@ -168,7 +175,7 @@ static inline char *fanotify_info_name2(struct fanotify_info *info)
>  static inline void fanotify_info_init(struct fanotify_info *info)
>  {
>  	BUILD_BUG_ON(FANOTIFY_FH_HDR_LEN + MAX_HANDLE_SZ > U8_MAX);
> -	BUILD_BUG_ON(NAME_MAX > U8_MAX);
> +	BUILD_BUG_ON(PATH_MAX > U16_MAX);
>  
>  	info->dir_fh_totlen = 0;
>  	info->dir2_fh_totlen = 0;
> @@ -214,8 +221,7 @@ static inline void fanotify_info_set_file_fh(struct fanotify_info *info,
>  static inline void fanotify_info_copy_name(struct fanotify_info *info,
>  					   const struct qstr *name)
>  {
> -	if (WARN_ON_ONCE(name->len > NAME_MAX) ||
> -	    WARN_ON_ONCE(info->name2_len > 0))
> +	if (name->len >= PATH_MAX || WARN_ON_ONCE(info->name2_len > 0))
>  		return;
>  
>  	info->name_len = name->len;
> @@ -225,7 +231,7 @@ static inline void fanotify_info_copy_name(struct fanotify_info *info,
>  static inline void fanotify_info_copy_name2(struct fanotify_info *info,
>  					    const struct qstr *name)
>  {
> -	if (WARN_ON_ONCE(name->len > NAME_MAX))
> +	if (name->len >= PATH_MAX)
>  		return;
>  
>  	info->name2_len = name->len;
> 
> base-commit: 165768bb70265b5c38cf0b73fafd75be235f8b14
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR