[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161029185222.GT19539@ZenIV.linux.org.uk>
Date: Sat, 29 Oct 2016 19:52:22 +0100
From: Al Viro <viro@...IV.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Christoph Hellwig <hch@....de>, Jan Kara <jack@...e.cz>,
Dmitry Monakhov <dmonakhov@...nvz.org>,
Jeff Moyer <jmoyer@...hat.com>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-aio@...ck.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
stable <stable@...r.kernel.org>
Subject: Re: [PATCH] aio: fix a user triggered use after free (and fix freeze
protection of aio writes)
On Sat, Oct 29, 2016 at 10:47:58AM -0700, Linus Torvalds wrote:
> Also, honestly, make it use a helper: "aio_file_start_write()" and
> "aio_file_end_write()" that has the comments and the lockdep games.
>
> Because that patch is just too effing ugly.
>
> Does something like the attached work for you guys?
No. The use-after-free problem is real, nasty and only papered over by
that patch.
What happens is that io_submit_one()
* allocates aio_kiocb
* does fget() and stuffs the struct file * into kiocb
* in case of early problems we call kiocb_free(), freeing kiocb and
doing fput() on file, then bugger off.
* otherwise, eventually we get to passing that iocb to
->read_iter()/->write_iter().
* if that has resulted in anything other than -EIOCBQUEUED, we
call aio_complete(), which calls kiocb_free(), freeing kiocb and doing fput()
on file.
* if ->{read,write}_iter() returns -EIOCBQUEUED, we expect
aio_complete() to be called asynchronously.
And that call can happen as soon as we return from __blockdev_direct_IO()
(even earlier, actually). As soon as that happens, the reference to
struct file we'd acquired in io_submit_one() is dropped. If descriptor
table had been shared, another thread might have already closed that sucker,
and fput() from aio_complete() would free struct file.
That's what this patch is papering over. Because if we hit that scenario
and struct file *does* get closed asynchronously just as our ->write_iter()
is returning from __blockdev_direct_IO(), we are fucked. Not only struct
file might be freed - struct inode might've been gone too. And a bunch
of ->write_iter/->read_iter instances do access struct inode after the call
of __blockdev_direct_IO(). file_write_end() is just the tip of the iceberg -
see examples I've posted today for the things we *can't* move around.
Powered by blists - more mailing lists