Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752184Ab1EPSXj (ORCPT ); Mon, 16 May 2011 14:23:39 -0400 Received: from smtp103.prem.mail.sp1.yahoo.com ([98.136.44.58]:42959 "HELO smtp103.prem.mail.sp1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1750787Ab1EPSXh (ORCPT ); Mon, 16 May 2011 14:23:37 -0400 X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- X-YMail-OSG: w8UOfpwVM1m6A4SGC6kcIBjzhy4vsQfpF4YAOTDQa6G2.3. .5l6UhtoiCUrqvEKjg.4T6I2wT7K8tMxlgnxwQOofywy88oHZfX7SWnqqCGf 98VU1LKzb9Q2pBxs0hVYr3rOYzew9p1sk2SqdSh_Eck76mYYhVm.3M3lHx4b pHEHy25u2SFsuMLQ3kKNlJ.9WlnF8AsMq9e1RhuWTyL7j4E0ANTegx.cT_26 BAzAy8FJeD.R5znWnTjwukvHi0FGZqufd3KB_RI19ev.EcaQYB3QmTx2D.u3 DseAEaGMr70RVRpXMjFJ.3IWCB6QZ47NXztzEsOQB2ukcPaEdi2OvXgol_ou tqxtLNaPX3l3mw5jIk_yKWqg23UN4xIabWivvi7A9_UMp63oBkfxdlp9QKM. EpUL7OpLsKuPWHMJCXID6bTZkkTkgCEKr.vjAo2L.8Rgeess3VpLIwYSY8Zz 231BKfEojT19rbW5SplFLJN.X0w-- X-Yahoo-Newman-Property: ymail-3 Message-ID: <4DD16B96.7020907@schaufler-ca.com> Date: Mon, 16 May 2011 11:23:18 -0700 From: Casey Schaufler User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.17) Gecko/20110414 Thunderbird/3.1.10 MIME-Version: 1.0 To: Steven Whitehouse CC: Mimi Zohar , linux-security-module@vger.kernl.org, cluster-devel@redhat.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, James Morris , David Safford , Andrew Morton , Greg KH , Dmitry Kasatkin , Mimi Zohar , Stephen Smalley , Eric Paris , Casey Schaufler Subject: Re: [PATCH v5 13/21] evm: add evm_inode_post_init call in gfs2 References: <1305557115-15652-1-git-send-email-zohar@linux.vnet.ibm.com> <1305557115-15652-14-git-send-email-zohar@linux.vnet.ibm.com> <1305559822.2855.14.camel@menhir> <1305561051.2669.10.camel@localhost.localdomain> <1305562469.2855.26.camel@menhir> <1305563758.2669.26.camel@localhost.localdomain> <1305568250.2669.47.camel@localhost.localdomain> <1305568671.2855.31.camel@menhir> In-Reply-To: <1305568671.2855.31.camel@menhir> X-Enigmail-Version: 1.1.1 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7106 Lines: 176 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 >>>>>>> --- >>>>>>> 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. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/