Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

Al,

any chance to send this user triggerable use after free on to Linus?

On Sun, Oct 16, 2016 at 07:51:22AM +0200, Christoph Hellwig wrote:
> From: Jan Kara <jack@xxxxxxx>
> 
> Currently we dropped freeze protection of aio writes just after IO was
> submitted. Thus aio write could be in flight while the filesystem was
> frozen and that could result in unexpected situation like aio completion
> wanting to convert extent type on frozen filesystem. Testcase from
> Dmitry triggering this is like:
> 
> for ((i=0;i<60;i++));do fsfreeze -f /mnt ;sleep 1;fsfreeze -u /mnt;done &
> fio --bs=4k --ioengine=libaio --iodepth=128 --size=1g --direct=1 \
>     --runtime=60 --filename=/mnt/file --name=rand-write --rw=randwrite
> 
> Fix the problem by dropping freeze protection only once IO is completed
> in aio_complete().
> 
> [hch: The above was the changelog of the original patch from Jan.
>  It turns out that it fixes something even more important - a use
>  after free of the file structucture given that the direct I/O
>  code calls fput and potentially drops the last reference to it in
>  aio_complete.  Together with two racing threads and a zero sized
>  I/O this seems easily exploitable]
> 
> Reported-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> [hch: switch to use __sb_writers_acquired and file_inode(file),
>       updated changelog]
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  fs/aio.c           | 28 +++++++++++++++++++++++++---
>  include/linux/fs.h |  1 +
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 1157e13..bf315cd 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1078,6 +1078,17 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
>  	unsigned tail, pos, head;
>  	unsigned long	flags;
>  
> +	if (kiocb->ki_flags & IOCB_WRITE) {
> +		struct file *file = kiocb->ki_filp;
> +
> +		/*
> +		 * Tell lockdep we inherited freeze protection from submission
> +		 * thread.
> +		 */
> +		__sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> +		file_end_write(file);
> +	}
> +
>  	/*
>  	 * Special case handling for sync iocbs:
>  	 *  - events go directly into the iocb for fast handling
> @@ -1460,13 +1471,24 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>  			return ret;
>  		}
>  
> -		if (rw == WRITE)
> +		if (rw == WRITE) {
>  			file_start_write(file);
> +			req->ki_flags |= IOCB_WRITE;
> +		}
> +
> +		if (rw == WRITE) {
> +			/*
> +			 * We release freeze protection in aio_complete(). Fool
> +			 * lockdep by telling it the lock got released so that
> +			 * it doesn't complain about held lock when we return
> +			 * to userspace.
> +			 */
> +			__sb_writers_release(file_inode(file)->i_sb,
> +					SB_FREEZE_WRITE);
> +		}
>  
>  		ret = iter_op(req, &iter);
>  
> -		if (rw == WRITE)
> -			file_end_write(file);
>  		kfree(iovec);
>  		break;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 16d2b6e..db600e9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -321,6 +321,7 @@ struct writeback_control;
>  #define IOCB_HIPRI		(1 << 3)
>  #define IOCB_DSYNC		(1 << 4)
>  #define IOCB_SYNC		(1 << 5)
> +#define IOCB_WRITE		(1 << 6)
>  
>  struct kiocb {
>  	struct file		*ki_filp;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---