linux-kernel - Re: [PATCH] aio: fix a user triggered use after free (and fix freeze protection of aio writes)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161030063222.GB1683@lst.de>
Date:   Sun, 30 Oct 2016 07:32:22 +0100
From:   Christoph Hellwig <hch@....de>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     Christoph Hellwig <hch@....de>, torvalds@...ux-foundation.org,
        jack@...e.cz, dmonakhov@...nvz.org, jmoyer@...hat.com,
        linux-fsdevel@...r.kernel.org, linux-aio@...ck.org,
        linux-kernel@...r.kernel.org, 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 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...

Yeah.

> 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.