Re: [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2
From: Mimi Zohar
Date: Mon May 16 2011 - 13:51:22 EST
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@xxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/