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

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

[Chengfeng Ye]

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

Copy the range position into the permission event and make ppos refer to
that event-owned value. 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: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 3 ++-
 fs/notify/fanotify/fanotify.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index a3555bebad63..c97d1be70310 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -601,7 +601,8 @@ static struct fanotify_event *fanotify_alloc_perm_event(const void *data,
 	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 : 0;
+	pevent->ppos = range ? &pevent->pos : NULL;
 	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..c964df6c0514 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -438,7 +438,8 @@ 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;
+	const loff_t *ppos;		/* &pos if range info is available */
 	size_t count;
 	u32 response;			/* userspace answer to the event */
 	unsigned short state;		/* state of the event */
-- 
2.43.0

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

[Jan Kara]

On Thu 30-07-26 16:58:01, 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
> 
> Copy the range position into the permission event and make ppos refer to
> that event-owned value. 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: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>

Thanks for report! That's indeed a nasty bug. FWIW I always disliked how we
passed pointer to pos around instead of pos itself only to be able to
indicate it is not defined. Since loff_t is actually signed, Amir, do you
remember why we didn't just define (loff_t)-1 as "range not provided"
message instead of passing around ppos?

I think getting rid of passing ppos around and storing it to the permission
event would be preferrable over the hybrid approach in this patch.

								Honza

> ---
>  fs/notify/fanotify/fanotify.c | 3 ++-
>  fs/notify/fanotify/fanotify.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index a3555bebad63..c97d1be70310 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -601,7 +601,8 @@ static struct fanotify_event *fanotify_alloc_perm_event(const void *data,
>  	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 : 0;
> +	pevent->ppos = range ? &pevent->pos : NULL;
>  	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..c964df6c0514 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -438,7 +438,8 @@ 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;
> +	const loff_t *ppos;		/* &pos if range info is available */
>  	size_t count;
>  	u32 response;			/* userspace answer to the event */
>  	unsigned short state;		/* state of the event */
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Chengfeng Ye]

Hi,

The v2 is just sent, thanks for the review!

Best, Chengfeng