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]

Date:   Sat, 29 Oct 2016 17:29:46 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Christoph Hellwig <hch@....de>
Cc:     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:

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

PS: I'm not saying that refcounting kiocb is the best solution - grabbing an
extra reference to struct file might be better (we have just dirtied that
cacheline, so the fact that struct file is shared more than kiocb shouldn't
matter much), but I really think that "file and everything attached to it
might disappear as soon as you get async IO started" is insanely brittle -
e.g. xfs_rw_iunlock(ip, iolock) in xfs_file_dio_aio_write() is also unsafe
<checks -next - yup, still there>

If struct file might be gone, so might struct inode and everything behind
it.  Which means that we either are not allowed to hold any locks across
__blockdev_direct_IO(), or need have end_io() callback taking care of
dropping those (and adjust the callers accordingly).  It might be not
impossible, but... *ouch*