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 18:57 +0100, Steven Whitehouse wrote:
> Hi,
> 
> On Mon, 2011-05-16 at 13:50 -0400, Mimi Zohar wrote:
> > 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
> > 
> 
> Yes, if that makes sense... I got the impression from the paper that
> there is the possibility of more xattrs being added in future and this
> way the fs end of things wouldn't have to change again when that
> happens. I'm still trying to get my head around it all, but it seems a
> cleaner solution to me - though I may well be missing something still,
> 
> Steve.

At this point at least, the only other xattr would be security.ima,
which isn't created/updated until __fput() is called.  

Your suggestion of security_inode_init_security() returning multiple
xattrs is a cleaner solution for EVM, but such a change requires the LSM
folks approval.

thanks,

Mimi