Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <54af4a92356090d88639531413ea8cb46837bd18.camel@linux.ibm.com>
Subject: Re: [RFC PATCH v2 2/3] fs: define a firmware security filesystem
 named fwsecurityfs
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     James Bottomley <James.Bottomley@HansenPartnership.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Nayna Jain <nayna@linux.ibm.com>
Cc:     linuxppc-dev@lists.ozlabs.org, linux-fsdevel@vger.kernel.org,
        linux-efi@vger.kernel.org,
        linux-security-module <linux-security-module@vger.kernel.org>,
        linux-kernel@vger.kernel.org,
        Michael Ellerman <mpe@ellerman.id.au>,
        Dov Murik <dovmurik@linux.ibm.com>,
        George Wilson <gcwilson@linux.ibm.com>, gjoyce@ibm.com,
        Matthew Garrett <mjg59@srcf.ucam.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>
Date:   Sun, 26 Jun 2022 11:48:06 -0400
In-Reply-To: <41ca51e8db9907d9060cc38adb59a66dcae4c59b.camel@HansenPartnership.com>
References: <20220622215648.96723-1-nayna@linux.ibm.com>
         <20220622215648.96723-3-nayna@linux.ibm.com> <YrQqPhi4+jHZ1WJc@kroah.com>
         <41ca51e8db9907d9060cc38adb59a66dcae4c59b.camel@HansenPartnership.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

On Thu, 2022-06-23 at 09:23 -0400, James Bottomley wrote:
> On Thu, 2022-06-23 at 10:54 +0200, Greg Kroah-Hartman wrote:
> [...]
> > > diff --git a/fs/fwsecurityfs/inode.c b/fs/fwsecurityfs/inode.c
> > > new file mode 100644
> > > index 000000000000..5d06dc0de059
> > > --- /dev/null
> > > +++ b/fs/fwsecurityfs/inode.c
> > > @@ -0,0 +1,159 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2022 IBM Corporation
> > > + * Author: Nayna Jain <nayna@linux.ibm.com>
> > > + */
> > > +
> > > +#include <linux/sysfs.h>
> > > +#include <linux/kobject.h>
> > > +#include <linux/fs.h>
> > > +#include <linux/fs_context.h>
> > > +#include <linux/mount.h>
> > > +#include <linux/pagemap.h>
> > > +#include <linux/init.h>
> > > +#include <linux/namei.h>
> > > +#include <linux/security.h>
> > > +#include <linux/lsm_hooks.h>
> > > +#include <linux/magic.h>
> > > +#include <linux/ctype.h>
> > > +#include <linux/fwsecurityfs.h>
> > > +
> > > +#include "internal.h"
> > > +
> > > +int fwsecurityfs_remove_file(struct dentry *dentry)
> > > +{
> > > +	drop_nlink(d_inode(dentry));
> > > +	dput(dentry);
> > > +	return 0;
> > > +};
> > > +EXPORT_SYMBOL_GPL(fwsecurityfs_remove_file);
> > > +
> > > +int fwsecurityfs_create_file(const char *name, umode_t mode,
> > > +					u16 filesize, struct dentry
> > > *parent,
> > > +					struct dentry *dentry,
> > > +					const struct file_operations
> > > *fops)
> > > +{
> > > +	struct inode *inode;
> > > +	int error;
> > > +	struct inode *dir;
> > > +
> > > +	if (!parent)
> > > +		return -EINVAL;
> > > +
> > > +	dir = d_inode(parent);
> > > +	pr_debug("securityfs: creating file '%s'\n", name);
> > 
> > Did you forget to call simple_pin_fs() here or anywhere else?
> > 
> > And this can be just one function with the directory creation file,
> > just check the mode and you will be fine.  Look at securityfs as an
> > example of how to make this simpler.
> 
> Actually, before you go down this route can you consider the namespace
> ramifications.  In fact we're just having to rework securityfs to pull
> out all the simple_pin_... calls because simple_pin_... is completely
> inimical to namespaces.
> 
> The first thing to consider is if you simply use securityfs you'll
> inherit all the simple_pin_... removal work and be namespace ready.  It
> could be that creating a new filesystem that can't be namespaced is the
> right thing to do here, but at least ask the question: would we ever
> want any of these files to be presented selectively inside containers? 
> If the answer is "yes" then simple_pin_... is the wrong interface.

Greg, the securityfs changes James is referring to are part of the IMA
namespacing patch set:
https://lore.kernel.org/linux-integrity/20220420140633.753772-1-stefanb@linux.ibm.com/

I'd really appreciate your reviewing the first two patches:
[PATCH v12 01/26] securityfs: rework dentry creation
[PATCH v12 02/26] securityfs: Extend securityfs with namespacing
support

thanks,

Mimi