Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752920Ab1EPRvT (ORCPT ); Mon, 16 May 2011 13:51:19 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:39815 "EHLO e38.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752066Ab1EPRvR (ORCPT ); Mon, 16 May 2011 13:51:17 -0400 Subject: Re: [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2 From: Mimi Zohar To: Steven Whitehouse Cc: linux-security-module@vger.kernl.org, cluster-devel@redhat.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, James Morris , David Safford , Andrew Morton , Greg KH , Dmitry Kasatkin , Mimi Zohar , Stephen Smalley , Eric Paris , Casey Schaufler In-Reply-To: <1305563758.2669.26.camel@localhost.localdomain> References: <1305557115-15652-1-git-send-email-zohar@linux.vnet.ibm.com> <1305557115-15652-14-git-send-email-zohar@linux.vnet.ibm.com> <1305559822.2855.14.camel@menhir> <1305561051.2669.10.camel@localhost.localdomain> <1305562469.2855.26.camel@menhir> <1305563758.2669.26.camel@localhost.localdomain> Content-Type: text/plain; charset="UTF-8" Date: Mon, 16 May 2011 13:50:50 -0400 Message-ID: <1305568250.2669.47.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 (2.30.3-1.fc13) Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6169 Lines: 160 On Mon, 2011-05-16 at 12:35 -0400, Mimi Zohar wrote: > On Mon, 2011-05-16 at 17:14 +0100, Steven Whitehouse wrote: > > Hi, > > > > On Mon, 2011-05-16 at 11:50 -0400, Mimi Zohar wrote: > > > On Mon, 2011-05-16 at 16:30 +0100, Steven Whitehouse wrote: > > > > Hi, > > > > > > > > On Mon, 2011-05-16 at 10:45 -0400, Mimi Zohar wrote: > > > > > After creating the initial LSM security extended attribute, call > > > > > evm_inode_post_init_security() to create the 'security.evm' > > > > > extended attribute. > > > > > > > > > > Signed-off-by: Mimi Zohar > > > > > --- > > > > > fs/gfs2/inode.c | 28 +++++++++++++++++++--------- > > > > > 1 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > [snip] > > > > > + struct xattr lsm_xattr; > > > > > + struct xattr evm_xattr; > > > > > > > > > > err = security_inode_init_security(&ip->i_inode, &dip->i_inode, qstr, > > > > > - &name, &value, &len); > > > > > + &lsm_xattr.name, &lsm_xattr.value, > > > > > + &lsm_xattr.value_len); > > > > > > > > > > if (err) { > > > > > if (err == -EOPNOTSUPP) > > > > > @@ -780,11 +781,20 @@ static int gfs2_security_init(struct gfs2_inode *dip, struct gfs2_inode *ip, > > > > > return err; > > > > > } > > > > > > > > > > - err = __gfs2_xattr_set(&ip->i_inode, name, value, len, 0, > > > > > - GFS2_EATYPE_SECURITY); > > > > > - kfree(value); > > > > > - kfree(name); > > > > > - > > > > > + err = __gfs2_xattr_set(&ip->i_inode, lsm_xattr.name, lsm_xattr.value, > > > > > + lsm_xattr.value_len, 0, GFS2_EATYPE_SECURITY); > > > > > + if (err < 0) > > > > > + goto out; > > > > > + err = evm_inode_post_init_security(&ip->i_inode, &lsm_xattr, > > > > > + &evm_xattr); > > > > > + if (err) > > > > > + goto out; > > > > > + err = __gfs2_xattr_set(&ip->i_inode, evm_xattr.name, evm_xattr.value, > > > > > + evm_xattr.value_len, 0, GFS2_EATYPE_SECURITY); > > > > > + kfree(evm_xattr.value); > > > > > +out: > > > > > + kfree(lsm_xattr.name); > > > > > + kfree(lsm_xattr.value); > > > > > return err; > > > > > } > > > > > > > > > > > > > Just wondering whether we could have a single call to the security > > > > subsystem which returns a vector of xattrs rather than having to call > > > > two different functions? > > > > > > > > Steve. > > > > > > There are a number of places that the LSM function is called immediately > > > followed by either EVM/IMA. In each of those places it is hidden from > > > the caller by calling the security_inode_XXX_security(). In this case > > > each fs has it's own method of creating an extended attribute. If that > > > method could be passed to security_inode_init_security, then > > > security_inode_init_security() could call both the LSM and EVM functions > > > directly. > > > > > > Mimi > > > > > > > I'm still not quite sure I understand... from a (very brief) look at the > > paper, it seems that what you are trying to do is add a new xattr to > > inodes which has some hash of some of the inode metadata (presumably > > including the selinux xattr and some other fields). > > Yes, for the time being the other metadata is i_ino, i_generation, > i_uid, i_gid, and i_mode. The IMA-appriasal extension would store the > file hash as an extended attribute. The digital-signature extension > would store a digitial signature instead of the hash. > > > I'm not sure why it matters whether the selinux data has been written to > > the buffers before the xattr containing the hash? The data will not > > change (I hope!) and if it does presumably the hash will pick that up > > when it is checked at a later date? > > In this case it doesn't matter, as there aren't any other xattrs at this > point. When the file closes, the file hash would be written out as > security.ima, causing security.evm to be updated to reflect the change. > > > The reason I'm asking is that currently the creation of GFS2 inodes is > > broken down into a number of transactions, carefully designed to ensure > > that the correct clean up occurs if there is an error. I would like to > > try and reduce the number of transactions during the create process > > where possible. That means I would like to move to a model which looks > > like this: > > > > 1. Calculate number of blocks required, based on inode + xattrs (if any) > > 2. Allocate blocks > > 3. Populate with data (i.e. set xattrs) > > > > I'm trying to work out whether there is some reason why we have to use > > your proposed: > > > > 1. Get selinux xattr > > 2. Set selinux xattr > > 3. Get EVM xattr > > 4. Set EVM xattr > > > > as opposed to getting all the xattrs in a single call and then being > > able to set them all in a single operation, if that makes sense? > > > > Steve. > > Yes, it makes sense. Just to clarify (and am cc'ing Stephen, Eric, and Casey). Instead of: int security_inode_init_security(struct inode *inode, struct inode *dir, const struct qstr *qstr, char **name, void **value, size_t *len); You're suggesting changing the interface to something like: int security_inode_init_security(struct inode *inode, struct inode *dir, const struct qstr *qstr, struct xattr **xattrs); where 'struct xattr' is defined as (9th patch): --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -70,6 +70,12 @@ struct xattr_handler { size_t size, int flags, int handler_flags); }; +struct xattr { + char *name; + void *value; + size_t value_len; +}; + ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t); ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t); ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size); xattrs would be null terminated. The fs would be responsible for freeing the xattrs? thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/