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

From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alexey Gladkov <gladkov.alexey@gmail.com>
Subject: Re: [GIT PULL] proc fixes v2 for v5.8-rc1
Date: Fri, 12 Jun 2020 17:47:34 -0500	[thread overview]
Message-ID: <875zbvnbp5.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <CAHk-=whEMmvh=gAgo=Ae0zaJ06vfaYrKxa3jV+AgPBz450Rerw@mail.gmail.com> (Linus Torvalds's message of "Fri, 12 Jun 2020 13:16:24 -0700")

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Fri, Jun 12, 2020 at 1:06 PM Eric W. Biederman <ebiederm@xmission.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.

Yeah.  I have a similar feeling.

Especially since it took about 7 years for someone to notice something
was not right.

Still right now I am not up to digging and adding warnings and causing
things to fail.  What energy I have I am going to use to keep sorting
out exec.  I am not ready to page back in the fine details of how all of
the filesystem pieces interact right now.

I am wondering now if there are any crazy corner cases of the unix
domain sockets where fsnotify could latch onto one, and have problems.
It looks like the inode comes from the underlying filesystem so we
should be safe.

Eric