[PATCH] direct_io: fix use after free in __blockdev_direct_IO

[PATCH] direct_io: fix use after free in __blockdev_direct_IO

[Xiaotian Feng]

dio might be freed in direct_io_worker, then check of dio->flags causes
a use after free bug, which results following kmemcheck warning:

[  247.720572] WARNING: kmemcheck: Caught 8-bit read from freed memory (ffff88022dd9b020)
[  247.720575] 0000000009000000010000000100000003000000000000000002000000000000
[  247.720593]  f f f f f f f f f f f f f f f f f f f f f f f f f f f f f f f f
[  247.720611]  ^
[  247.720612]
[  247.720616] Pid: 0, comm: swapper Not tainted 2.6.32 #7 0M860N/OptiPlex 760
[  247.720618] RIP: 0010:[<ffffffff81160f76>]  [<ffffffff81160f76>] __blockdev_direct_IO+0xae6/0xc10
[  247.720626] RSP: 0018:ffff88022dd0fbe8  EFLAGS: 00010286
[  247.720628] RAX: ffff88000ae13540 RBX: ffff88022dd9b000 RCX: ffff88022dd9d400
[  247.720630] RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000286
[  247.720632] RBP: ffff88022dd0fcc8 R08: 0000000000000000 R09: ffff88022ddbb000
[  247.720634] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000200
[  247.720637] R13: 0000000000000000 R14: ffff88022dd9b108 R15: 0000000000000000
[  247.720640] FS:  0000000000000000(0000) GS:ffff88000ae00000(0000) knlGS:0000000000000000
[  247.720642] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  247.720644] CR2: ffff88021cfc9218 CR3: 000000022d843000 CR4: 00000000000406f0
[  247.720647] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  247.720649] DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
[  247.720651]  [<ffffffff8115e229>] blkdev_direct_IO+0x49/0x50
[  247.720655]  [<ffffffff810e415b>] generic_file_aio_read+0x62b/0x660
[  247.720659]  [<ffffffff8112feb2>] do_sync_read+0xd2/0x110
[  247.720663]  [<ffffffff811303b5>] vfs_read+0xb5/0x1a0
[  247.720666]  [<ffffffff8113095c>] sys_read+0x4c/0x80
[  247.720669]  [<ffffffff81009f02>] system_call_fastpath+0x16/0x1b
[  247.720674]  [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nikanth Karthikesan <knikanth@suse.de>
Cc: Zach Brown <zach.brown@oracle.com>
---
 fs/direct-io.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 4012885..de1ad3d 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -935,7 +935,7 @@ static ssize_t
 direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode, 
 	const struct iovec *iov, loff_t offset, unsigned long nr_segs, 
 	unsigned blkbits, get_block_t get_block, dio_iodone_t end_io,
-	struct dio *dio)
+	struct dio *dio, bool *dio_freed)
 {
 	unsigned long user_addr; 
 	unsigned long flags;
@@ -944,6 +944,7 @@ direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode,
 	ssize_t ret2;
 	size_t bytes;
 
+	*dio_freed = false;
 	dio->inode = inode;
 	dio->rw = rw;
 	dio->blkbits = blkbits;
@@ -1081,6 +1082,7 @@ direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode,
 	if (ret2 == 0) {
 		ret = dio_complete(dio, offset, ret);
 		kfree(dio);
+		*dio_freed = true;
 	} else
 		BUG_ON(ret != -EIOCBQUEUED);
 
@@ -1113,6 +1115,7 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
 	int flags)
 {
 	int seg;
+	bool dio_freed;
 	size_t size;
 	unsigned long addr;
 	unsigned blkbits = inode->i_blkbits;
@@ -1197,7 +1200,11 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
 		(end > i_size_read(inode)));
 
 	retval = direct_io_worker(rw, iocb, inode, iov, offset,
-				nr_segs, blkbits, get_block, end_io, dio);
+				  nr_segs, blkbits, get_block, end_io,
+				  dio, &dio_freed);
+
+	if (dio_freed)
+		goto out;
 
 	/*
 	 * In case of error extending write may have instantiated a few
-- 
1.6.5.2

Re: [PATCH] direct_io: fix use after free in __blockdev_direct_IO

[Al Viro]

On Thu, Dec 17, 2009 at 04:49:32PM +0800, Xiaotian Feng wrote:
> @@ -1197,7 +1200,11 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
>  		(end > i_size_read(inode)));
>  
>  	retval = direct_io_worker(rw, iocb, inode, iov, offset,
> -				nr_segs, blkbits, get_block, end_io, dio);
> +				  nr_segs, blkbits, get_block, end_io,
> +				  dio, &dio_freed);
> +
> +	if (dio_freed)
> +		goto out;

Um... I'm not sure that this would be the right fix.  How about simple
s/dio->flags/flags/ in the line below?

Re: [PATCH] direct_io: fix use after free in __blockdev_direct_IO

[Xiaotian Feng]

On 12/17/2009 05:29 PM, Al Viro wrote:
> On Thu, Dec 17, 2009 at 04:49:32PM +0800, Xiaotian Feng wrote:
>> @@ -1197,7 +1200,11 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
>>   		(end>  i_size_read(inode)));
>>
>>   	retval = direct_io_worker(rw, iocb, inode, iov, offset,
>> -				nr_segs, blkbits, get_block, end_io, dio);
>> +				  nr_segs, blkbits, get_block, end_io,
>> +				  dio,&dio_freed);
>> +
>> +	if (dio_freed)
>> +		goto out;
>
> Um... I'm not sure that this would be the right fix.  How about simple
> s/dio->flags/flags/ in the line below?
Yes, dio->flags is not changed in direct_io_worker(), your method is 
better, thanks.