Re: [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)

On Sun, Mar 3, 2019 at 6:36 PM Al Viro [off-list ref] wrote:

OK, having dug through the archives, the reasons were not strong.
So that part is OK...

I've committed the patch.

However, I didn't actually do the separate and independent cleanups:

quoted

@@ -1060,6 +1071,8 @@ static inline void iocb_put(struct aio_kiocb *iocb)
 {
      if (refcount_read(&iocb->ki_refcnt) == 0 ||
          refcount_dec_and_test(&iocb->ki_refcnt)) {
+             if (iocb->ki_filp)
+                     fput(iocb->ki_filp);
              percpu_ref_put(&iocb->ki_ctx->reqs);
              kmem_cache_free(kiocb_cachep, iocb);
      }

That reminds me - refcount_t here is rather ridiculous; what we
have is
        * everything other than aio_poll: ki_refcnt is 0 all along
        * aio_poll: originally 0, then set to 2, then two iocb_put()
are done (either both synchronous to aio_poll(), or one sync and one
async).

That's not a refcount at all.  It's a flag, set only for aio_poll ones.
And that test in iocb_put() is "if flag is set, clear it and bugger off".

What's worse, AFAICS the callers in aio_poll() are buggered (see below)

Ok. Suggestions?

quoted

-     if (unlikely(apt.error)) {
-             fput(req->file);
+     if (unlikely(apt.error))
              return apt.error;
-     }

      if (mask)
              aio_poll_complete(aiocb, mask);

Looking at that thing...  How does it manage to avoid leaks
when we try to use it on e.g. /dev/tty, which has
        poll_wait(file, &tty->read_wait, wait);
        poll_wait(file, &tty->write_wait, wait);
in n_tty_poll()?

I don't think that's he only case that uses multiple poll entries.
It's now the poll() machinery is designed to be used, after all.
Although maybe everybody ends up using just a single wait-queue..

AFAICS, we'll succeed adding it to the first queue, then have
aio_poll_queue_proc() fail and set apt.error to -EINVAL.

Yeah, that's bogus, but documented. I guess nobody really expects to
use aio_poll on anything but a socket or something?

But your refcount leak looks valid. Hmm.

So yes, that whole ki_refcnt looks a bit odd and pointless. And
apparently buggy too.

Comments?

Can we just

 (a) remove ki_refcnt entirely

 (b) remove the "iocb_put(aiocb);"

from aio_poll()?

Every actual successful io submission should end in aio_complete(), or
we free the aio iocb in the "out_put_req" error case. There's no point
in doing the refcount as you pointed out, and it seems to be actively
buggy.

Anyway, all of this (and your suggestion to just remove
"aio_poll_complete()" and just use "aio_complete()") is independent of
the file refcounting part, I feel. Which is why I just committed that
patch as-is (84c4e1f89fef "aio: simplify - and fix - fget/fput for
io_submit()").

                   Linus