Re: [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

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 <zohar@us.ibm.com>
> > > > > ---
> > > > >  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