Re: [PATCH v2] fanotify: fix use-after-free of file range info

On Thu 30-07-26 21:43:16, Chengfeng Ye wrote:
> fsnotify_pre_content() builds its file_range on the triggering task's
> stack. fanotify_alloc_perm_event() saves a pointer to range.pos in the
> heap-allocated permission event so copy_range_info_to_user() can report
> the offset later.
> 
> The event reader can set the event state to FAN_EVENT_REPORTED and then
> sleep while preparing the file descriptor. If a signal interrupts the
> triggering task at that point, fanotify_get_response() changes the state
> to FAN_EVENT_CANCELED and returns. This unwinds the file_range stack
> frame while the reader still owns the event. The reader then dereferences
> pevent->ppos and copies the stale stack value to userspace.
> 
> KASAN reported:
> 
>   BUG: KASAN: use-after-free in fanotify_read+0x293e/0x2970
>   Read of size 8 at addr ffff88811434fc50 by task fanotify_inotif/95
>   Call Trace:
>    fanotify_read+0x293e/0x2970
>    vfs_read+0x177/0xa20
>    ksys_read+0xf7/0x1c0
>    do_syscall_64+0xf9/0x540
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Store the range position directly in the permission event and use
> FANOTIFY_NO_RANGE when range information is unavailable. The event remains
> alive until the reader finishes, so the reported offset no longer depends
> on the triggering task's stack.
> 
> Fixes: 870499bc1d4d ("fanotify: report file range info with pre-content events")
> Cc: [email protected]
> Suggested-by: Jan Kara <[email protected]>
> Signed-off-by: Chengfeng Ye <[email protected]>

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

								Honza

> ---
> 
> Changes in v2:
> - Remove ppos from fanotify_perm_event and use FANOTIFY_NO_RANGE as the
>   sentinel for unavailable range information, as suggested by Jan Kara.
> - Read the event-owned position directly when reporting range information.
> 
> Link: https://lore.kernel.org/linux-fsdevel/[email protected]/ [v1]
> 
>  fs/notify/fanotify/fanotify.c      | 3 +--
>  fs/notify/fanotify/fanotify.h      | 6 ++++--
>  fs/notify/fanotify/fanotify_user.c | 4 ++--
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index a3555bebad63..b05b6d3abb87 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -600,8 +600,7 @@ static struct fanotify_event *fanotify_alloc_perm_event(const void *data,
>  	pevent->hdr.len = 0;
>  	pevent->state = FAN_EVENT_INIT;
>  	pevent->path = *path;
> -	/* NULL ppos means no range info */
> -	pevent->ppos = range ? &range->pos : NULL;
> +	pevent->pos = range ? range->pos : FANOTIFY_NO_RANGE;
>  	pevent->count = range ? range->count : 0;
>  	path_get(path);
>  
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index a0619e7694d5..3710543dbf82 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -428,6 +428,8 @@ FANOTIFY_ME(struct fanotify_event *event)
>  	return container_of(event, struct fanotify_mnt_event, fae);
>  }
>  
> +#define FANOTIFY_NO_RANGE	((loff_t)-1)
> +
>  /*
>   * Structure for permission fanotify events. It gets allocated and freed in
>   * fanotify_handle_event() since we wait there for user response. When the
> @@ -438,7 +440,7 @@ FANOTIFY_ME(struct fanotify_event *event)
>  struct fanotify_perm_event {
>  	struct fanotify_event fae;
>  	struct path path;
> -	const loff_t *ppos;		/* optional file range info */
> +	loff_t pos;			/* FANOTIFY_NO_RANGE if unavailable */
>  	size_t count;
>  	u32 response;			/* userspace answer to the event */
>  	unsigned short state;		/* state of the event */
> @@ -468,7 +470,7 @@ static inline bool fanotify_event_has_access_range(struct fanotify_event *event)
>  	if (!(event->mask & FANOTIFY_PRE_CONTENT_EVENTS))
>  		return false;
>  
> -	return FANOTIFY_PERM(event)->ppos;
> +	return FANOTIFY_PERM(event)->pos != FANOTIFY_NO_RANGE;
>  }
>  
>  static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse)
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index b604e3da58ad..bba92d691f0d 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -675,12 +675,12 @@ static size_t copy_range_info_to_user(struct fanotify_event *event,
>  	if (WARN_ON_ONCE(info_len > count))
>  		return -EFAULT;
>  
> -	if (WARN_ON_ONCE(!pevent->ppos))
> +	if (WARN_ON_ONCE(pevent->pos == FANOTIFY_NO_RANGE))
>  		return -EINVAL;
>  
>  	info.hdr.info_type = FAN_EVENT_INFO_TYPE_RANGE;
>  	info.hdr.len = info_len;
> -	info.offset = *(pevent->ppos);
> +	info.offset = pevent->pos;
>  	info.count = pevent->count;
>  
>  	if (copy_to_user(buf, &info, info_len))
> -- 
> 2.43.0
-- 
Jan Kara <[email protected]>
SUSE Labs, CR