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  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:   Sun, 3 Mar 2019 11:44:33 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>,
        Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jason Baron <jbaron@...mai.com>, kgraul@...ux.ibm.com,
        ktkhai@...tuozzo.com, kyeongdon.kim@....com,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>, pabeni@...hat.com,
        syzkaller-bugs@...glegroups.com, xiyou.wangcong@...il.com,
        Christoph Hellwig <hch@....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)

On Sun, Mar 3, 2019 at 7:18 AM Al Viro <viro@...iv.linux.org.uk> 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'm assuming you're talking about the second vfs_poll() in
aio_poll_complete_work()? The one we call before we check for
"rew->cancelled" properly under the spinlock?

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

So honestly, the whole filp handling in aio looks overly complicated to me.

All the different operations do that silly fget/fput() dance, although
aio_read/write at least tried to make a common helper function for
handling it.

But as far as I can tell, they *all* could do:

 - look up file in aio_submit() when allocating and creating the aio_kiocb

 - drop the filp in 'iocb_put()' (which happens whether things
complete successfully or not).

and we'd have avoided a lot of complexity, and we'd have avoided this bug.

Your patch fixes the poll() case, but it does so by just letting the
existing complexity remain, and adding a second fget/fput pair in the
poll logic.

It would seem like it would be much better to rip all the complexity
out entirely, and replace it with sane, simple and obviously correct
code.

Hmm?

In other words, why wouldn't something like the attached work instead?

TOTALLY UNTESTED! It builds, and it looks sane, but maybe I'm
overlooking some obvious problem with it. But doesn't it look nice to
see

 2 files changed, 41 insertions(+), 50 deletions(-)

with actual code reduction, and a fundamental simplification in
handling of the file pointer?

                 Linus

View attachment "patch.diff" of type "text/x-patch" (7944 bytes)

Powered by blists - more mailing lists