Message-ID: <20211209154703.4mprhv2rcgvgkmx5@wittgenstein>
Date:   Thu, 9 Dec 2021 16:47:03 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Stefan Berger <stefanb@...ux.ibm.com>
Cc:     linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
        serge@...lyn.com, containers@...ts.linux.dev,
        dmitry.kasatkin@...il.com, ebiederm@...ssion.com,
        krzysztof.struczynski@...wei.com, roberto.sassu@...wei.com,
        mpeters@...hat.com, lhinds@...hat.com, lsturman@...hat.com,
        puiterwi@...hat.com, jejb@...ux.ibm.com, jamjoom@...ibm.com,
        linux-kernel@...r.kernel.org, paul@...l-moore.com, rgb@...hat.com,
        linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, Dec 09, 2021 at 10:00:59AM -0500, Stefan Berger wrote:
> 
> On 12/9/21 09:41, Christian Brauner wrote:
> > On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote:
> > > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> > > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > > > > Move the dentries into the ima_namespace for reuse by virtualized
> > > > > SecurityFS. Implement function freeing the dentries in order of
> > > > > files and symlinks before directories.
> > > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
> > > > > ---
> > > > This doesn't work as implemented, I think.
> > > > 
> > > > What I would have preferred and what I tried to explain in the earlier
> > > > review was:
> > > > Keep the dentry stashing global since it is only needed for init_ima_ns.
> > > > Then struct ima_namespace becomes way smaller and simpler.
> > > > If you do that then it makes sense to remove the additional dget() in
> > > > securityfs_create_dentry() for non-init_ima_ns.
> > > > Then you can rely on auto-cleanup in .kill_sb() or on
> > > > ima_securityfs_init() failure and you only need to call
> > > > ima_fs_ns_free_dentries() if ns != init_ima_ns.
> > s/ns != init_ima_ns/ns == init_ima_ns/
> > 
> > > > IIuc, it seems you're currently doing one dput() too many since you're
> > > > calling securityfs_remove() in the error path for non-init_ima_ns which
> > > > relies on the previous increased dget() which we removed.
> 
> I thought that securityfs_remove() will now simply influence when a dentry
> is removed and freed. If we call it in the error cleanup path in
> non-init_user_ns case it would go away right there and leave nothing to do
> for .kill_sb() while an additional dget() would require the cleanup as well
> but do another cleanup then in .kill_sb() since that brings the reference
> count to 0 via the dput()s that it does. Am I wrong on this?

With your change you get one dget() from lookup_one_len() in
securityfs_create_dentry() for non-init_ima_ns. That's added to the
dcache via d_instantiate().
If you call securityfs_dentry_remove() in the error path or anywhere
else it does:

	dir = d_inode(dentry->d_parent);
	inode_lock(dir);
	if (simple_positive(dentry)) {
		if (d_is_dir(dentry))
			simple_rmdir(dir, dentry);
		else
			simple_unlink(dir, dentry);
		dput(dentry);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That dput() right there is for the additional dget() in
securityfs_create_dentry() but we didn't take that. So the dput() is one
too many now since simple_rmdir() and simple_unlink() will have consumed
one already. (You should be able to easily see this if you compile with
sanitizers on and let your init function fail somewhere in the middle.)

(What usually should happen is sm like this:

void binderfs_remove_file(struct dentry *dentry)
{
	struct inode *parent_inode;

	parent_inode = d_inode(dentry->d_parent);
	inode_lock(parent_inode);
	if (simple_positive(dentry)) {
		dget(dentry);
		simple_unlink(parent_inode, dentry);
		d_delete(dentry);
		dput(dentry);
	}
	inode_unlock(parent_inode);
})

> 
> 
> > > If you really want to move the dentry stashing into struct ima_namespace
> > > even though it's really unnecessary then you may as well not care about
> > > the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns)
> > > call in .kill_sb(). But I really think not dragging dentry stashing into
> > > struct ima_namespace is the correct way to go about this.
> 
> 
> I moved the dentries into the ima_namespace so that each namespace holds a
> pointer to the dentries it owns and isolates them. We certainly wouldn't
> want to have IMA namespaces write over the current static variables and
> create a mess with what these are pointing to ( https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L359
> ) and possible race conditions when doing parallel initialization (if that's
> possible at all). This also reduces the code size and we don't need two
> different implementations for init_user_ns and non-init_user_ns. So I don't
> quite understand whey we wouldn't want to have the dentries isolated via
> ima_namespace?

My point was this:
Afaict, nowhere in ima are the stashed dentries needed apart from
ima_policy_release() which is the .release method of the
file_operations where the policy dentry is removed.

The dentries only exist because for pre-namespaced ima if you created a
dentry going through securityfs_create_dentry() you're pinning the
super_block of init-securityfs via simple_pin_fs(). That obliges you to
call securityfs_remove() later on in order to call simple_unpin_fs() for
all dentries.

But for namespaced-ima with namespaced-securityfs there is no more call
to simple_{pin,unpin}_fs(). Consequently you don't need to stash the
dentries anywhere to have them available for removal later on. They will
be automatically cleaned up during .kill_sb().

The one exception I was unaware of reading the code before is in
ima_policy_release(). So apologies, I didn't see that. There you remove:

static int ima_release_policy(struct inode *inode, struct file *file)
{
	struct ima_namespace *ns = get_current_ns();
	const char *cause = ns->valid_policy ? "completed" : "failed";

	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
		return seq_release(inode, file);

	if (ns->valid_policy && ima_check_policy(ns) < 0) {
		cause = "failed";
		ns->valid_policy = 0;
	}

	pr_info("policy update %s\n", cause);
	integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
			    "policy_update", cause, !ns->valid_policy, 0);

	if (!ns->valid_policy) {
		ima_delete_rules(ns);
		ns->valid_policy = 1;
		clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
		return 0;
	}

	ima_update_policy(ns);
#if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
	securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
	ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;

^^^^^^^^^^^^^^^^^^^^^^^^^

But even so, why then stash all those dentries if the only dentry that
you ever remove while ima is active - and ima isn't a module so can't be
unloaded - is the IMAFS_DENTRY_IMA_POLICY. Simply stash the single
dentry in struct ima_namespace and forget about all the other ones and
avoid wasting memory. But maybe I'm misunderstanding something.

I'm going to get my booster shot and hopefully I'll be able to work
tomorrow and later today but I wouldn't bet on it.

Christian

Powered by blists - more mailing lists