Message-ID: <5650aa13ef875e70d8a87696c4e871f8a4d574a1.camel@linux.ibm.com>
Subject: Re: [RFC v2 19/19] ima: Setup securityfs for IMA namespace
From:   James Bottomley <jejb@linux.ibm.com>
Reply-To: jejb@linux.ibm.com
To:     Christian Brauner <christian.brauner@ubuntu.com>,
        Stefan Berger <stefanb@linux.ibm.com>
Cc:     linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        serge@hallyn.com, containers@lists.linux.dev,
        dmitry.kasatkin@gmail.com, ebiederm@xmission.com,
        krzysztof.struczynski@huawei.com, roberto.sassu@huawei.com,
        mpeters@redhat.com, lhinds@redhat.com, lsturman@redhat.com,
        puiterwi@redhat.com, jamjoom@us.ibm.com,
        linux-kernel@vger.kernel.org, paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
Date:   Mon, 06 Dec 2021 09:21:15 -0500
In-Reply-To: <20211206141108.evjrqsmmgpjp3ias@wittgenstein>
References: <20211203023118.1447229-1-stefanb@linux.ibm.com>
         <20211203023118.1447229-20-stefanb@linux.ibm.com>
         <df433bc52ca1e0408d48bbace4c34a573991f5ba.camel@linux.ibm.com>
         <6306b4e5-f26d-1704-6344-354eb5387abf@linux.ibm.com>
         <20211206141108.evjrqsmmgpjp3ias@wittgenstein>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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:
> > On 12/3/21 12:03, James Bottomley wrote:
[...]
> > > > +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