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

[Casey Schaufler <casey@schaufler-ca.com>]

On 5/16/2011 10:57 AM, 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,

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.

> Steve.
>
>
>