On Mon, 2021-12-06 at 15:42 +0100, Christian Brauner wrote:

On Mon, Dec 06, 2021 at 09:21:15AM -0500, James Bottomley wrote:

quoted

On Mon, 2021-12-06 at 15:11 +0100, Christian Brauner wrote:

quoted

On Fri, Dec 03, 2021 at 01:06:13PM -0500, Stefan Berger wrote:

quoted

On 12/3/21 12:03, James Bottomley wrote:

[...]

quoted

quoted

quoted

quoted

+int ima_fs_ns_init(struct ima_namespace *ns)
+{
+	ns->mount = securityfs_ns_create_mount(ns->user_ns);

 
This actually triggers on the call to
securityfs_init_fs_context,
but nothing happens because the callback is null.  Every
subsequent use of fscontext will trigger this.  The point of
a
keyed supeblock is that fill_super is only called once per
key,
that's the place we should be doing this.   It should also
probably be a blocking notifier so any consumer of securityfs
can
be namespaced by registering for this notifier.

What I don't like about the fill_super is that it gets called
too
early:

[   67.058611] securityfs_ns_create_mount @ 102 target user_ns:
ffff95c010698c80; nr_extents: 0
[   67.059836] securityfs_fill_super @ 47  user_ns:
ffff95c010698c80;
nr_extents: 0

We are switching to the target user namespace in
securityfs_ns_create_mount.  The expected nr_extents at this
point
is 0, since user_ns hasn't been configured, yet. But then
security_fill_super is also called with nr_extents 0. We cannot
use
that, it's too early!

So the problem is that someone could mount securityfs before any
idmappings are setup or what?

Yes, not exactly: we put a call to initialize IMA in
create_user_ns()
but it's too early to have the mappings, so we can't create the
securityfs entries in that call.  We need the inode to pick up the
root
owner from the s_user_ns mappings, so we can't create the dentries
for
the IMA securityfs entries until those mappings exist.

I'm assuming that by the time someone tries to mount securityfs
inside
the namespace, the mappings are set up, which is why triggering the
notifier to add the files on first mount seems like the best place
to
put it.

quoted

 How does moving the setup to a later stage help at all? I'm
struggling to make sense of this.

It's not moving all the setup, just the creation of the securityfs
entries.

quoted

 When or even if idmappings are written isn't under imas control.
Someone could mount securityfs without any idmappings setup. In
that
case they should get what they deserve, everything owner by
overflowuid/overflowgid, no?

Right, in the current scheme of doing things, if they still haven't
written the mappings by the time they do the mount, they're just
going
to get nobody/nogroup as uid/gid, but that's their own fault.

quoted

Or you can require in fill_super that kuid 0 and kgid 0 are
mapped
and fail if they aren't.

We can't create the securityfs entries in fill_super ... I already
tried and the locking just won't allow it.  And if we create them
ahead

What is the locking issue there exactly?

The main problem is we have no vfsmount and we can't create one in
there because the fill super is triggered by the vfsmount creation for
the actual mount.  It's all done under the sb->s_umount semaphore.

I'm looking at ima_fs_ns_late_init() and there's nothing there that
would cause obvious issues. You might not be able to use
securityfs_create_*() in there for some reason but that just means
you need to add a simple helper. Nearly every filesystem that needs
to pre-create files does it in fill_super. So I really fail to see
what the issue is currently. I mist just miss something obvious.

I think we might get it to work if we keep the root dentry in the
securityfs namespace entries instead of the vfsmount; I'll investigate.

James