linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: davem@davemloft.net, jbaron@akamai.com, kgraul@linux.ibm.com,
	ktkhai@virtuozzo.com, kyeongdon.kim@lge.com,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	pabeni@redhat.com, syzkaller-bugs@googlegroups.com,
	xiyou.wangcong@gmail.com, hch@lst.de
Subject: [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)
Date: Sun, 3 Mar 2019 15:18:47 +0000	[thread overview]
Message-ID: <20190303151846.GQ2217@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20190303135502.GP2217@ZenIV.linux.org.uk>

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

  reply	other threads:[~2019-03-03 15:19 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   ` Al Viro [this message]
2019-03-03 18:37     ` [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll) 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     ` [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll) Dmitry Vyukov

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=20190303151846.GQ2217@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --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=xiyou.wangcong@gmail.com \
    --subject='[PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)' \
    /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

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