Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20230928130147.564503-1-mszeredi@redhat.com> <20230928130147.564503-5-mszeredi@redhat.com>
 <CAHC9VhQD9r+Qf5Vz1XmxUdJJJO7HNTKdo8Ux=n+xkxr=JGFMrw@mail.gmail.com>
 <CAJfpegsPbDgaz46x4Rr9ZgCpF9rohVHsvuWtQ5LNAdiYU_D4Ww@mail.gmail.com>
 <a25f2736-1837-f4ca-b401-85db24f46452@themaw.net> <CAJfpegv78njkWdaShTskKXoGOpKAndvYYJwq7CLibiu+xmLCvg@mail.gmail.com>
 <CAHC9VhTwnjhfmkT5Rzt+SBf-8hyw4PYkbuPYnm6XLoyY7VAUiw@mail.gmail.com>
In-Reply-To: <CAHC9VhTwnjhfmkT5Rzt+SBf-8hyw4PYkbuPYnm6XLoyY7VAUiw@mail.gmail.com>
From:   Miklos Szeredi <miklos@szeredi.hu>
Date:   Fri, 6 Oct 2023 10:53:47 +0200
Message-ID: <CAJfpegsZqF4TnnFBsV-tzi=w_7M=To5DeAjyW=cei9YuG+qMfg@mail.gmail.com>
Subject: Re: [PATCH v3 4/4] add listmount(2) syscall
To:     Paul Moore <paul@paul-moore.com>
Cc:     Ian Kent <raven@themaw.net>, Miklos Szeredi <mszeredi@redhat.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-api@vger.kernel.org, linux-man@vger.kernel.org,
        linux-security-module@vger.kernel.org, Karel Zak <kzak@redhat.com>,
        David Howells <dhowells@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <christian@brauner.io>,
        Amir Goldstein <amir73il@gmail.com>,
        Matthew House <mattlloydhouse@gmail.com>,
        Florian Weimer <fweimer@redhat.com>,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 6 Oct 2023 at 04:56, Paul Moore <paul@paul-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.

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.

> 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.

Thanks,
Miklos