[lkml]   [2016]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] aio: fix a user triggered use after free (and fix freeze protection of aio writes)
On Sat, Oct 29, 2016 at 05:12:30PM +0100, Al Viro wrote:
> Eww... IOW, as soon as we'd submitted an async iocb, nobody can so much as
> look at struct file *or* iocb, right? Or underlying inode, or any fs-private
> data structures attached to it...


> I certainly agree that it's a bug, but IMO you are just papering over it.
> Just look at e.g.
> written = mapping->a_ops->direct_IO(iocb, &data);
> /*
> * Finally, try again to invalidate clean pages which might have been
> * cached by non-direct readahead, or faulted in by get_user_pages()
> * if the source of the write was an mmap'ed region of the file
> * we're writing. Either one is a pretty crazy thing to do,
> * so we don't support it 100%. If this invalidation
> * fails, tough, the write still worked...
> */
> if (mapping->nrpages) {
> invalidate_inode_pages2_range(mapping,
> pos >> PAGE_SHIFT, end);
> }
> in generic_file_direct_write() - who said that mapping doesn't point into
> freed memory by that point?

True, somewhat unlike due to inode caching, but for sure possible
and something that needs to be deal with.

> Why do we play that kind of insane games, anyway? Why not e.g. refcount
> the (async) iocb and have kiocb_free() drop the reference, with io_submit_one()
> holding one across the call of aio_run_iocb()? Cacheline bouncing issues?
> Anything more subtle?

There shouldn't really be a need to refcount the iocb itself - nothing
worth looking at. The one we consider was struct file, and I didn't
like it because of the cacheline bouncing if we decrement if from both
the submitter and completion context. But it starts to sounds like
the sane version more and more.

 \ /
  Last update: 2016-10-30 07:32    [W:0.104 / U:0.968 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site