linux-kernel - Re: [GIT PULL] proc fixes v2 for v5.8-rc1

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]

Message-ID: <CAHk-=whEMmvh=gAgo=Ae0zaJ06vfaYrKxa3jV+AgPBz450Rerw@mail.gmail.com>
Date:   Fri, 12 Jun 2020 13:16:24 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexey Gladkov <gladkov.alexey@...il.com>
Subject: Re: [GIT PULL] proc fixes v2 for v5.8-rc1

On Fri, Jun 12, 2020 at 1:06 PM Eric W. Biederman <ebiederm@...ssion.com> wrote:
>
> I have a sense that a use after free that anyone can trigger could be a
> bit dangerous, and despite not being the only virtual filesystem in the
> kernel proc is the only virtual filesystem that called new_inode_pseudo.

So the reason I pulled that change despite my questions was that I do
agree with the whole "there's probably little point to use
new_inode_pseudo() here" argument.

But at the same time, I ghet the feeling that this partly just is
papering over the problem. If fsnotify causes problems with a
new_inode_pseudo() inode, then fsnotify should be _checking_ for that
case.

And if fsnotify were to check for it, then the reason for /proc to use
it would largely go away. Maybe the debug check for umount matters,
but honestly it doesn't really seem to be a big deal.

A pseudo-inode is basically independent of the filesystem it was
mounted as, so the generic_shutdown_super() check not triggering for
them is sensible, I feel.

But yeah, we could also make the rule go the other way, and simply
make sure that "new_inode_pseudo()" itself checks that the super-block
you give it is something fundamenally unmountable and was created with
'kern_mount()'.

That would have also figured out that the /proc case was broken.

So the main objection I have here is really that this fix feels
incomplete, and doesn't really reflect the underlying issue, just
fixes the symptom.

Either the underlying issue is that you shouldn't call 'fsnotify' on
/proc, or the underlying issue is that /proc was using a bad inode and
nobody even noticed until the fsnotify issue.

This is not a huge deal. I think you've fixed the bug, I just have
this itch that the thing that triggered it shouldn't have happened in
the first place.

             Linus