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: <20161029162946.GS19539@ZenIV.linux.org.uk>
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*

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ