Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata <kamatam@amazon.com>]

On Sat, 2023-12-09 10:35:01 -0800, Casey Schaufler wrote:
>
> On 12/9/2023 10:08 AM, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 7:24 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 12/8/2023 3:32 PM, Paul Moore wrote:
> >>> On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> On 12/8/2023 2:43 PM, Paul Moore wrote:
> >>>>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> >>>>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> >>>>> ..
> >>>>>
> >>>>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> >>>>>>> at the top.  However, before we go too much further on this, can we
> >>>>>>> get clarification that Casey was able to reproduce this on a stock
> >>>>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> >>>>>>> this problem on Linux v6.5.
> >>>>>>>
> >>>>>>> However, for the moment I'm going to assume this is a real problem, is
> >>>>>>> there some reason why the existing pid_revalidate() code is not being
> >>>>>>> called in the bind mount case?  From what I can see in the original
> >>>>>>> problem report, the path walk seems to work okay when the file is
> >>>>>>> accessed directly from /proc, but fails when done on the bind mount.
> >>>>>>> Is there some problem with revalidating dentrys on bind mounts?
> >>>>>> Hi Paul,
> >>>>>>
> >>>>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> >>>>>>
> >>>>>> After reading this thread, I have doubt about solving this in VFS.
> >>>>>> Honestly, however, I'm not sure if it's entirely relevant today.
> >>>>> Have you tried simply mounting proc a second time instead of using a bind mount?
> >>>>>
> >>>>>  % mount -t proc non /new/location/for/proc
> >>>>>
> >>>>> I ask because from your description it appears that proc does the
> >>>>> right thing with respect to revalidation, it only becomes an issue
> >>>>> when accessing proc through a bind mount.  Or did I misunderstand the
> >>>>> problem?
> >>>> It's not hard to make the problem go away by performing some simple
> >>>> action. I was unable to reproduce the problem initially because I
> >>>> checked the Smack label on the bind mounted proc entry before doing
> >>>> the cat of it. The problem shows up if nothing happens to update the
> >>>> inode.
> >>> A good point.
> >>>
> >>> I'm kinda thinking we just leave things as-is, especially since the
> >>> proposed fix isn't something anyone is really excited about.
> >> "We have to compromise the performance of our sandboxing tool because of
> >> a kernel bug that's known and for which a fix is available."
> >>
> >> If this were just a curiosity that wasn't affecting real development I
> >> might agree. But we've got a real world problem, and I don't see ignoring
> >> it as a good approach. I can't see maintainers of other LSMs thinking so
> >> if this were interfering with their users.
> > While the reproducer may be written for Smack, there are plenty of
> > indications that this applies to all LSMs and my comments have taken
> > that into account.
> >
> > If you're really that upset, try channeling that outrage into your
> > editor and draft a patch for this that isn't awful.
> 
> We could "just" wait for the lsm_set_self_attr() syscall to land, and
> suggest that it be used instead of the buggy /proc interfaces.
> 
> I would love to propose a patch that's less sucky, but have not come
> up with one. My understanding of VFS internals isn't up to the task,
> I fear.

As an another option, perhaps adding an even stricter hidepid mode in
procfs (and avoid bind mount) could be reasonable?