Date:	Wed, 03 Dec 2008 13:17:16 -0500
From:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
To:	Dave Hansen <dave@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	James Morris <jmorris@...ei.org>,
	Christoph Hellwig <hch@...radead.org>,
	Al Viro <viro@...IV.linux.org.uk>,
	David Safford <safford@...son.ibm.com>,
	Serge Hallyn <serue@...ux.vnet.ibm.com>,
	Mimi Zohar <zohar@...ibm.com>
Subject: Re: [PATCH 3/6] integrity: IMA as an integrity service provider

On Tue, 2008-12-02 at 15:35 -0800, Dave Hansen wrote: 
> On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> > index 0000000..6c6fcd9
> > --- /dev/null
> > +++ b/security/integrity/ima/Kconfig
> > @@ -0,0 +1,34 @@
> > +#
> > +# IBM Integrity Measurement Architecture
> > +#
> > +config IMA
> > +	bool "Integrity Measurement Architecture(IMA)"
> > +	depends on INTEGRITY
> > +	depends on ACPI
> > +	select SECURITYFS
> > +	select CRYPTO
> > +	select CRYPTO_HMAC
> > +	select CRYPTO_MD5
> > +	select CRYPTO_SHA1
> > +	select TCG_TPM
> > +	select TCG_TIS
> > +	help
> > +	  The Trusted Computing Group(TCG) runtime Integrity
> > +	  Measurement Architecture(IMA) maintains a list of hash
> > +	  values of executables and other sensitive system files
> > +	  loaded into the run-time of this system.  If your system
> > +	  has a TPM chip, then IMA also maintains an aggregate
> > +	  integrity value over this list inside the TPM hardware.
> > +	  These measurements and the aggregate (signed inside the
> > +	  TPM) can be retrieved and presented to remote parties to
> > +	  establish system properties. If unsure, say N.
> 
> This still doesn't tell me how it helps me.  "If an attacker managed to
> change the contents of an important system file being measured, we can
> tell."  Right?

Yes, that is a clearer description. Thanks.

> > +config IMA_MEASURE_PCR_IDX
> > +	int "PCR for Aggregate (8 <= Index <= 14)"
> > +	depends on IMA
> > +	range 8 14
> > +	default 10
> > +	help
> > +	  IMA_MEASURE_PCR_IDX determines the TPM PCR register index
> > +	  that IMA uses to maintain the integrity aggregate of the
> > +	  measurement list.  If unsure, use the default 10.
> 
> Why would you want to change this?  Can it be done at runtime instead of
> compile time?  I don't know what a PCR is.

The only reason to change it would be if in the future, TCG decides on a
standard PCR for IMA, other than 10, or if they pick 10 for something
else. We really don't need a runtime variable for this, but kconfig
makes it easy to change once if necessary in the future.

> > +#define ima_printk(level, format, arg...)		\
> > +	printk(level "ima (%s): " format, __func__, ## arg)
> > +
> > +#define ima_error(format, arg...)	\
> > +	ima_printk(KERN_ERR, format, ## arg)
> > +
> > +#define ima_info(format, arg...)	\
> > +	ima_printk(KERN_INFO, format, ## arg)
> 
> Please don't.  Can you use pr_debug() and friends?

will clean this up.

> > +/* digest size for IMA, fits SHA1 or MD5 */
> > +#define IMA_DIGEST_SIZE		20
> 
> When another algorithm (with a longer digest) is added, will we detect
> that without this just plain breaking?
>  

As the kernel command line option "ima_hash=" verifies the crypto
algorithm specified, changes to the code would be required to support a
new algorithm anyway.

> > +struct ima_h_table {
> > +	atomic_long_t len;	/* number of stored measurements in the list */
> > +	atomic_long_t violations;
> > +	unsigned int max_htable_size;
> > +	struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
> > +	atomic_t queue_len[IMA_MEASURE_HTABLE_SIZE];
> > +};
> > +extern struct ima_h_table ima_htable;
> > +
> > +static inline unsigned long IMA_HASH_KEY(u8 *digest)
> > +{
> > +	return (hash_long(*digest, IMA_HASH_BITS));
> > +}
> 
> 'return' isn't a function. :)

> Also, please lower-case this sucker so we know it isn't a macro.

Ok

> > +void ima_add_violation(struct inode *inode, const unsigned char *filename,
> > +		       char *op, char *cause)
> > +{
> > +	int result, namelen;
> > +	struct ima_inode_measure_entry measure_entry;
> > +	struct ima_store_template_data template = {
> > +		.name = "ima",
> > +		.len = sizeof(measure_entry),
> > +		.data = (char *)&measure_entry,
> > +		.violation = 1,
> > +	};
> 
> If '.data' is a char*, perhaps it should be a void*.  If it already is a
> void*, you don't need a cast. 
> 
> > +int ima_must_measure(void *data)
> > +{
> > +	struct ima_measure_data *mdata = (struct ima_measure_data *)data;
> 
> No need to cast a void*.  You have several of these.  Please find all of
> them and fix them up.

Thank you. will be in the next set patches.

> > +	struct ima_iint_cache *iint;
> > +	int must_measure = -EACCES;
> > +
> > +	if (!S_ISREG(mdata->inode->i_mode))
> > +		return -EPERM;
> > +	if ((mdata->mask & MAY_WRITE) || (mdata->mask & MAY_APPEND))
> > +		return -EPERM;
> > +
> > +	rcu_read_lock();
> > +	iint = ima_iint_lookup(mdata->inode);
> > +	if (iint)
> > +		kref_get(&iint->refcount);
> > +	rcu_read_unlock();
> 
> All of ima_iint_lookup()'s callers do the exact same thing.  Please just
> make it ima_iint_find_get(), and do the RCU and kref_get() internally
> and once.

cleaner, thanks.

> > +	if (!iint) {
> > +		int rc;
> > +
> > +		/* Most insertions are done at inode_alloc,
> > +		 * except those allocated before late_initcall.
> > +		 * Can't initialize at security_initcall,
> > +		 * got to wait at least until proc_init.
> > +		 */
> > +		rc = ima_iint_insert(mdata->inode);
> > +		if (rc < 0)
> > +			return rc;
> > +
> > +		rcu_read_lock();
> > +		iint = ima_iint_lookup(mdata->inode);
> > +		if (!iint) {
> > +			rcu_read_unlock();
> > +			return -ENOMEM;
> > +		}
> > +		kref_get(&iint->refcount);
> > +		rcu_read_unlock();
> > +	}
> 
> How about a retry goto instead of just copying the code again?  Better
> yet, can you just stick all of this in a helper function?

After the ima_iint_find_get() recommendation above, this becomes a lot
smaller.

> > +int ima_iint_insert(struct inode *inode)
> > +{
> > +	struct ima_iint_cache *iint;
> > +	int rc = 0;
> > +
> > +	iint = kzalloc(sizeof(*iint), GFP_KERNEL);
> 
> Does this basically get done for every inode, or only special ones?  I
> just wonder if having a dedicated slab with a constructor to do
> redundant things like mutex_init() would be helpful.

every inode, except those allocated before init_latecall.

> > +static void ima_add_boot_aggregate(void)
> > +{
> > +	struct ima_inode_measure_entry measure_entry;
> > +	struct ima_store_template_data template = {
> > +		.name = "ima",
> > +		.len = sizeof(measure_entry),
> > +		.data = (char *)&measure_entry,
> > +	};
> > +	int namelen, result;
> > +
> > +	memset(&measure_entry, 0, sizeof measure_entry);
> > +	namelen = strlen(boot_aggregate_name);
> > +	if (namelen > IMA_EVENT_NAME_LEN_MAX)
> > +		namelen = IMA_EVENT_NAME_LEN_MAX;
> > +	memcpy(measure_entry.file_name, boot_aggregate_name, namelen);
> > +
> > +	if (ima_used_chip) {
> > +		int i;
> > +		u8 pcr_i[IMA_DIGEST_SIZE];
> > +		struct hash_desc desc;
> > +		struct crypto_hash *tfm;
> > +		struct scatterlist sg;
> 
> All of this stack stuff with very important, large sounding names makes
> me nervous.  Can you reassure me?

The crypto code here will be moved to ima_crypto.c and will be
refactored, cleaning up the code. Both measure_entry and template could
be allocated/freed each time, but does that make sense?

> > +		tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
> > +		if (!tfm || IS_ERR(tfm)) {
> > +			ima_error("error initializing digest.\n");
> > +			return;
> > +		}
> > +		desc.tfm = tfm;
> > +		desc.flags = 0;
> > +		crypto_hash_init(&desc);
> > +
> > +		/* cumulative sha1 over tpm registers 0-7 */
> > +		for (i = 0; i < 8; i++) {
> 
> Surely there's a NR_TPM_REGISTERS or similar somewhere.

Will look.

> > +static void ima_file_free(struct file *file)
> > +{
> > +	struct inode *inode = file->f_dentry->d_inode;
> > +	struct ima_iint_cache *iint;
> > +
> > +	if (S_ISDIR(inode->i_mode))
> > +		return;
> > +	if ((file->f_mode & FMODE_WRITE) &&
> > +	    (atomic_read(&inode->i_writecount) == 1)) {
> > +		rcu_read_lock();
> > +		iint = ima_iint_lookup(inode);
> > +		if (!iint) {
> > +			rcu_read_unlock();
> > +			return;
> > +		}
> > +		kref_get(&iint->refcount);
> > +		rcu_read_unlock();
> > +
> > +		mutex_lock(&iint->mutex);
> > +		if (iint->version != inode->i_version)
> > +			iint->flags &= ~IMA_MEASURED;
> > +		mutex_unlock(&iint->mutex);
> > +		kref_put(&iint->refcount, iint_free);
> > +	}
> > +}
> 
> I'm also wondering if there's a way to wrap up the mutex operations
> since this seems to be done the exact same way every time.  Dunno, maybe
> it is too much locking obfuscation for just a few lines saved.

Unlike the ima_iint_lookup(), the code within the mutex locking differs
between calls.

> > +static int ima_path_check_integrity(struct path *path, int mask)
> > +{
> > +	struct ima_measure_data mdata;
> > +
> > +	memset(&mdata, 0, sizeof mdata);
> > +	mdata.inode = path->dentry->d_inode;
> > +	mdata.mask = mask;
> > +	mdata.function = PATH_CHECK;
> > +
> > +	/* Invalidate PCR, if a measured file is already open for read */
> > +	if ((mdata.mask & (MAY_WRITE | MAY_READ)) == MAY_WRITE) {
> 
> It would warm my heart to see something like this:
> 
> int mdata_is_write_only(struct ima_measure_data *mdata)
> {
> 	if (mdata.mask & MAY_READ)
> 		return 0;
> 	return mdata.mask & MAY_WRITE;
> }
> 
> I don't know about you, but I find that immeasurably nicer.  Is it even
> right?

In addition to MAY_READ/MAY_WRITE, there might be other flags.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists