[lkml]   [2016]   [Oct]   [29]   [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:20:17PM +0200, Christoph Hellwig wrote:
> On Sat, Oct 29, 2016 at 01:24:51PM +0100, Al Viro wrote:
> > How about taking this chunk (i.e. telling lockdep that we are not holding this
> > thing) past the iter_op() call, where file_end_write() used to be?
> We can't as that would not fix the use after free (at least for the lockdep
> case - otherwise the call is a no-op). Once iter_op returns aio_complete
> might have dropped our reference to the file, and another thread might
> have closed the fd so that the fput from aio_complete was the last one.
> This is something that xfstests/323 can reproduce under the right conditions.

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) {
pos >> PAGE_SHIFT, end);
in generic_file_direct_write() - who said that mapping doesn't point into
freed memory by that point? Or that
ret = generic_write_checks(iocb, from);
if (ret > 0)
ret = __generic_file_write_iter(iocb, from);
in generic_file_write_iter() won't blow up on inode_unlock() of a freed inode?
Or that anything of that sort won't happen in other ->write_iter instances,
for that matter?

NAK, with apologies for not having looked at that earlier. The bug is real,
all right, but this is not a solution - both incomplete and far too brittle.

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?

 \ /
  Last update: 2016-10-29 18:13    [W:0.061 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site