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

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

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

 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.

 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.

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
of time, that create of the entries will trigger fill_super because we
need the superblock to hang the dentries off.

James