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  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:   Mon, 4 Mar 2019 13:22:44 -0800
From:   Linus Torvalds <>
To:     Al Viro <>
Cc:     Eric Dumazet <>,
        David Miller <>,
        Jason Baron <>,,,,
        Linux List Kernel Mailing <>,
        Netdev <>,,,,
        Christoph Hellwig <>
Subject: 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 <> 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:

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

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

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


Powered by blists - more mailing lists