Date:   Fri, 6 Oct 2023 19:07:52 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     Ian Kent <raven@...maw.net>, Miklos Szeredi <mszeredi@...hat.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-api@...r.kernel.org, linux-man@...r.kernel.org,
        linux-security-module@...r.kernel.org, Karel Zak <kzak@...hat.com>,
        David Howells <dhowells@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <christian@...uner.io>,
        Amir Goldstein <amir73il@...il.com>,
        Matthew House <mattlloydhouse@...il.com>,
        Florian Weimer <fweimer@...hat.com>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v3 4/4] add listmount(2) syscall

On Fri, Oct 6, 2023 at 4:53 AM Miklos Szeredi <miklos@...redi.hu> wrote:
> On Fri, 6 Oct 2023 at 04:56, Paul Moore <paul@...l-moore.com> wrote:
>
> > > Also I cannot see the point in hiding some mount ID's from the list.
> > > It seems to me that the list is just an array of numbers that in
> > > itself doesn't carry any information.
> >
> > I think it really comes down to the significance of the mount ID, and
> > I can't say I know enough of the details here to be entirely
> > comfortable taking a hard stance on this.  Can you help me understand
> > the mount ID concept a bit better?
>
> Mount ID is a descriptor that allows referring to a specific struct
> mount from userspace.
>
> The old 32 bit mount id is allocated with IDA from a global pool.
> Because it's non-referencing it doesn't allow uniquely identifying a
> mount.  That was a design mistake that I made back in 2008, thinking
> that the same sort of dense descriptor space as used for file
> descriptors would work.  Originally it was used to identify the mount
> and the parent mount in /proc/PID/mountinfo.  Later it was also added
> to the following interfaces:
>
>  - name_to_handle_at(2) returns 32 bit value
>  - /proc/PID/FD/fdinfo
>  - statx(2) returns 64 bit value
>
> It was never used on the kernel interfaces as an input argument.

Thanks for the background.

> statmount(2) and listmount(2) require the mount to be identified by
> userspace, so having a unique ID is important.  So the "[1/4] add
> unique mount ID" adds a new 64 bit ID (still global) that is allocated
> sequentially and only reused after reboot.   It is used as an input to
> these syscalls.  It is returned by statx(2) if requested by
> STATX_MNT_ID_UNIQUE and as an array of ID's by listmount(2).
>
> I can see mild security problems with the global allocation, since a
> task can observe mounts being done in other namespaces.  This doesn't
> sound too serious, and the old ID has similar issues.  But I think
> making the new ID be local to the mount namespace is also feasible.

The LSM hook API is designed to operate independently from any of the
kernel namespaces; while some LSMs may choose to be aware of
namespaces and adjust their controls accordingly, there is no
requirement that they do so.  For that reason, I'm not too bothered
either way if the mount ID is global or tied to a namespace.

> > While I'm reasonably confident that we want a security_sb_statfs()
> > control point in statmount(), it may turn out that we don't want/need
> > a call in the listmount() case.  Perhaps your original patch was
> > correct in the sense that we only want a single security_sb_statfs()
> > call for the root (implying that the child mount IDs are attributes of
> > the root/parent mount)?  Maybe it's something else entirely?
>
> Mounts are arranged in a tree (I think it obvious how) and
> listmount(2) just lists the IDs of the immediate children of a mount.
>
> I don't see ID being an attribute of a mount, it's a descriptor.

In this case I think the approach you took originally in this thread
is likely what we want, call security_sb_statfs() against the root
mount in listmount().  Just please move it after the capability
checks.

If you look at the two LSMs which implement the security_sb_statfs(),
Smack and SELinux, you see that Smack treats this as a read operation
between the current process and the specified mount and SELinux treats
this as a filesystem:getattr operations between the current process
and the specified mount.  In both cases I can see that being the right
approach for reading a list of child mounts off of a root mount.

Does that sound good?  I'm guessing that's okay since that was how you
wrote it in your original patch, but there has been a lot of
discussion since then :)

-- 
paul-moore.com

Powered by blists - more mailing lists