Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue

[Siddh Raman Pant <code@siddh.me>]

Hello Dipanjan,

It would be nice if you could test this patch and tell if it fixes the
issue on v5.10, as you had reported it earlier.

Please apply the following commits before applying this patch:
db8facfc9faf ("watch_queue, pipe: Free watchqueue state after clearing pipe ring")
353f7988dd84 ("watchqueue: make sure to serialize 'wqueue->defunct' properly")

I have tested locally on tag v5.10, using the reproducer available on
syzkaller dashboard. The crash occurred when the patches weren't applied,
and it no longer occurs after applying the three patches.

Thanks,
Siddh

> If not done, a reference to a freed pipe remains in the watch_queue,
> as this function is called before freeing a pipe in free_pipe_info()
> (see line 834 of fs/pipe.c).
> 
> This causes a UAF when post_one_notification() tries to access the pipe
> on a key update, which is reported by syzbot.
> 
> We also need to use READ_ONCE() in post_one_notification() to prevent the
> compiler from optimising and loading a non-NULL value from wqueue->pipe.
> 
> Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc
> Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com
> 
> Signed-off-by: Siddh Raman Pant <code@siddh.me>
> ---
> Changes in v3:
> - Restore the original unlock order, and clear before unlock.
> - Use READ_ONCE() in post path.
> 
> This was explained by David Howells <dhowells@redhat.com> in
> reply to v1. Not added Suggested-by since he didn't reply yet.
> 
> Changes in v2:
> - Removed the superfluous ifdef guard.
> 
>  kernel/watch_queue.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
> index bb9962b33f95..617425e34252 100644
> --- a/kernel/watch_queue.c
> +++ b/kernel/watch_queue.c
> @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
>  				  struct watch_notification *n)
>  {
>  	void *p;
> -	struct pipe_inode_info *pipe = wqueue->pipe;
> +	struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);
>  	struct pipe_buffer *buf;
>  	struct page *page;
>  	unsigned int head, tail, mask, note, offset, len;
> @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue)
>  		spin_lock_bh(&wqueue->lock);
>  	}
>  
> +	/* Clearing the watch queue, so we should clean the associated pipe. */
> +	if (wqueue->pipe) {
> +		wqueue->pipe->watch_queue = NULL;
> +		wqueue->pipe = NULL;
> +	}
> +
>  	spin_unlock_bh(&wqueue->lock);
>  	rcu_read_unlock();
>  }
> -- 
> 2.35.1