From: Dmitry Vyukov <dvyukov@google.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
David Miller <davem@davemloft.net>,
Jason Baron <jbaron@akamai.com>,
kgraul@linux.ibm.com, Kirill Tkhai <ktkhai@virtuozzo.com>,
kyeongdon kim <kyeongdon.kim@lge.com>,
LKML <linux-kernel@vger.kernel.org>,
netdev <netdev@vger.kernel.org>, Paolo Abeni <pabeni@redhat.com>,
syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)
Date: Mon, 4 Mar 2019 08:53:38 +0100 [thread overview]
Message-ID: <CACT4Y+ZquWnN+8OOxFobnr-371evgnvgQvfJzhFZg4ZP+Gd-wQ@mail.gmail.com> (raw)
In-Reply-To: <20190303151846.GQ2217@ZenIV.linux.org.uk>
On Sun, Mar 3, 2019 at 4:19 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Mar 03, 2019 at 01:55:02PM +0000, Al Viro wrote:
>
> > Maybe unrelated to this bug, but... What's to prevent a wakeup
> > that happens just after we'd been added to a waitqueue by ->poll()
> > triggering aio_poll_wake(), which gets to aio_poll_complete()
> > with its fput() *before* we'd reached the end of ->poll() instance?
> >
> > I wonder if adding
> > get_file(req->file); // make sure that early completion is safe
> > right after
> > req->file = fget(iocb->aio_fildes);
> > if (unlikely(!req->file))
> > return -EBADF;
> > paired with
> > fput(req->file);
> > right after out: in aio_poll() is needed... Am I missing something
> > obvious here? Christoph?
>
> In more details - normally IOCB_CMD_POLL handling looks so:
>
> 1) io_submit(2) allocates aio_kiocb instance and passes it to aio_poll()
> 2) aio_poll() resolves the descriptor to struct file by
> req->file = fget(iocb->aio_fildes)
> 3) aio_poll() sets ->woken to false and raises ->ki_refcnt of that
> aio_kiocb to 2 (bumps by 1, that is).
> 4) aio_poll() calls vfs_poll(). After sanity checks (basically, "poll_wait()
> had been called and only once") it locks the queue. That's what the extra
> reference to iocb had been for - we know we can safely access it.
> 5) With queue locked, we check if ->woken has already been set to true
> (by aio_poll_wake()) and, if it had been, we unlock the queue, drop
> a reference to aio_kiocb and bugger off - at that point it's
> a responsibility to aio_poll_wake() and the stuff called/scheduled by
> it. That code will drop the reference to file in req->file, along
> with the other reference to our aio_kiocb.
> 6) otherwise, we see whether we need to wait. If we do, we unlock
> the queue, drop one reference to aio_kiocb and go away - eventual
> wakeup (or cancel) will deal with the reference to file and with the
> other reference to aio_kiocb
> 7) otherwise we remove ourselves from waitqueue (still under the queue
> lock), so that wakeup won't get us. No async activity will be happening,
> so we can safely drop req->file and iocb ourselves.
>
> If wakeup happens while we are in vfs_poll(), we are fine - aio_kiocb
> won't get freed under us, so we can do all the checks and locking safely.
> And we don't touch ->file if we detect that case.
>
> However, vfs_poll() most certainly *does* touch the file it had been
> given. So wakeup coming while we are still in ->poll() might end up
> doing fput() on that file. That case is not too rare, and usually
> we are saved by the still present reference from descriptor table -
> that fput() is not the final one. But if another thread closes that
> descriptor right after our fget() and wakeup does happen before
> ->poll() returns, we are in trouble - final fput() done while we
> are in the middle of a method.
>
> What we need is to hold on to the file reference the same way we do with
> aio_kiocb. No need to store the reference to what we'd got in a separate
> variable - req->file is never reassigned and we'd already made sure that
> req won't be freed under us, so dropping the extra reference with
> fput(req->file) is fine in all cases.
>
> Fixes: bfe4037e722ec
> Cc: stable@vger.kernel.org
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Please add the Reported-by tag from the report.
If you don't add it, then either:
1. somebody (you) will need to remember to later go and associate fix
the the report with "#syz fix" command
2. or the bug will stay open and syzbot will never report
use-after-frees in unix_dgram_poll again (it's still the same already
reported bug)
3. or worse somebody will spend time re-debugging this bug just to
find that you already fixed it but did not include the tag
Thanks
> ---
> diff --git a/fs/aio.c b/fs/aio.c
> index 3083180a54c8..7e88bfabdac2 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1767,6 +1767,7 @@ static ssize_t aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb)
>
> /* one for removal from waitqueue, one for this function */
> refcount_set(&aiocb->ki_refcnt, 2);
> + get_file(req->file);
>
> mask = vfs_poll(req->file, &apt.pt) & req->events;
> if (unlikely(!req->head)) {
> @@ -1793,6 +1794,7 @@ static ssize_t aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb)
> spin_unlock_irq(&ctx->ctx_lock);
>
> out:
> + fput(req->file);
> if (unlikely(apt.error)) {
> fput(req->file);
> return apt.error;
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20190303151846.GQ2217%40ZenIV.linux.org.uk.
> For more options, visit https://groups.google.com/d/optout.
prev parent reply other threads:[~2019-03-04 7:53 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-03 10:22 KASAN: use-after-free Read in unix_dgram_poll syzbot
2019-03-03 13:55 ` Al Viro
2019-03-03 15:18 ` [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll) Al Viro
2019-03-03 18:37 ` Eric Dumazet
2019-03-03 19:44 ` Linus Torvalds
2019-03-03 20:13 ` Linus Torvalds
2019-03-03 20:30 ` Al Viro
2019-03-03 22:23 ` Linus Torvalds
2019-03-04 2:36 ` Al Viro
2019-03-04 21:22 ` Linus Torvalds
2019-03-07 0:03 ` [PATCH 1/8] aio: make sure file is pinned Al Viro
2019-03-07 0:03 ` [PATCH 2/8] aio_poll_wake(): don't set ->woken if we ignore the wakeup Al Viro
2019-03-07 2:18 ` Al Viro
2019-03-08 11:16 ` zhengbin (A)
2019-03-07 0:03 ` [PATCH 3/8] aio_poll(): sanitize the logics after vfs_poll(), get rid of leak on error Al Viro
2019-03-07 2:11 ` zhengbin (A)
2019-03-07 0:03 ` [PATCH 4/8] aio_poll(): get rid of weird refcounting Al Viro
2019-03-07 0:03 ` [PATCH 5/8] make aio_read()/aio_write() return int Al Viro
2019-03-07 0:03 ` [PATCH 6/8] move dropping ->ki_eventfd into iocb_put() Al Viro
2019-03-07 0:03 ` [PATCH 7/8] deal with get_reqs_available() in aio_get_req() itself Al Viro
2019-03-07 0:03 ` [PATCH 8/8] aio: move sanity checks and request allocation to io_submit_one() Al Viro
2019-03-07 0:23 ` [PATCH 1/8] aio: make sure file is pinned Linus Torvalds
2019-03-07 0:41 ` Al Viro
2019-03-07 0:48 ` Al Viro
2019-03-07 1:20 ` Al Viro
2019-03-07 1:30 ` Linus Torvalds
2019-03-08 3:36 ` Al Viro
2019-03-08 15:50 ` Christoph Hellwig
2019-03-10 7:06 ` Al Viro
2019-03-10 7:08 ` [PATCH 1/8] pin iocb through aio Al Viro
2019-03-10 7:08 ` [PATCH 2/8] keep io_event in aio_kiocb Al Viro
2019-03-11 19:43 ` Christoph Hellwig
2019-03-11 21:17 ` Al Viro
2019-03-10 7:08 ` [PATCH 3/8] aio: store event at final iocb_put() Al Viro
2019-03-11 19:44 ` Christoph Hellwig
2019-03-11 21:13 ` Al Viro
2019-03-11 22:52 ` Al Viro
2019-03-10 7:08 ` [PATCH 4/8] Fix aio_poll() races Al Viro
2019-03-11 19:58 ` Christoph Hellwig
2019-03-11 21:06 ` Al Viro
2019-03-12 19:18 ` Christoph Hellwig
2019-03-10 7:08 ` [PATCH 5/8] make aio_read()/aio_write() return int Al Viro
2019-03-11 19:44 ` Christoph Hellwig
2019-03-10 7:08 ` [PATCH 6/8] move dropping ->ki_eventfd into iocb_destroy() Al Viro
2019-03-11 19:46 ` Christoph Hellwig
2019-03-10 7:08 ` [PATCH 7/8] deal with get_reqs_available() in aio_get_req() itself Al Viro
2019-03-11 19:46 ` Christoph Hellwig
2019-03-10 7:08 ` [PATCH 8/8] aio: move sanity checks and request allocation to io_submit_one() Al Viro
2019-03-11 19:48 ` Christoph Hellwig
2019-03-11 21:12 ` Al Viro
2019-03-11 19:41 ` [PATCH 1/8] pin iocb through aio Christoph Hellwig
2019-03-11 19:41 ` [PATCH 1/8] aio: make sure file is pinned Christoph Hellwig
2019-03-04 7:53 ` Dmitry Vyukov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACT4Y+ZquWnN+8OOxFobnr-371evgnvgQvfJzhFZg4ZP+Gd-wQ@mail.gmail.com \
--to=dvyukov@google.com \
--cc=davem@davemloft.net \
--cc=hch@lst.de \
--cc=jbaron@akamai.com \
--cc=kgraul@linux.ibm.com \
--cc=ktkhai@virtuozzo.com \
--cc=kyeongdon.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).