[Cluster-devel] [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2

[Cluster-devel] [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2

[Steven Whitehouse]

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.

[Cluster-devel] [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2

[Steven Whitehouse]

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).

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?

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.

[Cluster-devel] [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2

[Steven Whitehouse]

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.

[Cluster-devel] [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2

[Steven Whitehouse]

Hi,

On Wed, 2011-05-18 at 20:55 -0400, Mimi Zohar wrote:
> On Mon, 2011-05-16 at 12:25 -0700, Casey Schaufler wrote: 
> > On 5/16/2011 11:48 AM, Mimi Zohar wrote:
> > > On Mon, 2011-05-16 at 11:23 -0700, Casey Schaufler wrote:
> 
> > >> There is a very real possibility that multiple concurrent LSMs will
> > >> be supported before too long. Smack already uses multiple attributes
> > >> (SMACK64, SMACK64EXEC) on a file. Getting all the attributes in a
> > >> single call could result in an interface that requires parsing a
> > >> string argument, and we all know how popular those are. Introducing
> > >> an interface that we know isn't going to accommodate this upcoming
> > >> direction does not seem prudent.
> > > I would think that Smack would benefit from Steven's suggestion of
> > > returning an array of xattrs. Without his suggestion, I'm not sure how
> > > you are, or planning on, initializing multiple xattrs from a single LSM,
> > > unless of course you're not using security_inode_init_security().
> > 
> > The good news is that Smack has one required attribute. The others
> > are for special purposes and will usually be absent. It is easy to
> > imagine an LSM that always uses multiple attributes on a given file.
> > 
> > Yes, the array of xattr structures makes sense for any one LSM,
> > but there still needs to be the potential for multiple calls for
> > the multiple LSM case. I can't see that going away without a radical
> > LSM restructuring.
> > 
> > > Multiple LSMs calling security_inode_init_security() will be an issue
> > > for EVM, as EVM assumes there is a single LSM xattr on which to base the
> > > initial hmac.
> > 
> > That is far from the biggest issue with multiple LSMs, but is definitely
> > something to worry about.
> 
> Ok. After thinking about this a bit more, moving
> evm_inode_init_security() into security_inode_init_security() only works
> for the single LSM and EVM case, but not for the multiple LSMs and EVM
> case, as the 'stacker' would call each LSM's
> security_inode_iint_security().  Having the 'stacker' return an array of
> xattrs would make sense and, at the same time, resolve the EVM issue. In
> evm_inode_post_init_security(), EVM could then walk the list of xattrs.
> 
> Mimi
> 
> 
> 
That sounds like a reasonable solution to me,

Steve.