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

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 linux-cve-announce 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 14:23:33 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        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 12:30 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Sun, Mar 03, 2019 at 11:44:33AM -0800, Linus Torvalds wrote:
> >
> > 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?
>
> No.  The first one, right in aio_poll().

Ok, they are both confusing. The lifetime of that filp is just not
clear, with the whole "it could have been completed asynchronously"
issue.

> I'll need to dig out the mail archive from last year, but IIRC this
> had been considered and there'd been non-trivial problems.  Give me
> an hour or so and I'll dig that out (there'd been many rounds of
> review, with long threads involved, some private, some on fsdevel).

Looking more at the patch, it still looks eminently sane to me.I fixed
the silly "ki_filp" thing you noticed (I thought we made fput() take
NULL, but you're right we don't), and simplified the patch to not do
the changes that aren't necessary.

I fixed the silly "filp can be NULL" issue you pointed out (I lazily
thought we allowed fput(NULL), but you're right, we definitely don't),
and simplified the patch to not do the unnecessary changes where we
can just access the file pointer multiple different ways.

And the resulting kernel boots fine, but I doubt I have anything that
actually uses io_submit(), so that doesn't mean much.

Slightly updated patch attached anyway, even smaller than before:

 2 files changed, 36 insertions(+), 44 deletions(-)

with several of the new lines being comments about that file pointer
placement in the union.

                Linus

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