Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Fri, 2008-11-14 at 14:15 -0800, Andrew Morton wrote: 
> On Wed, 12 Nov 2008 22:47:12 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > This version resolves the merge issues resulting from the removal
> > of the nameidata parameter to inode_permission(), by moving the
> > integrity_inode_permission() call from inode_permission() to
> > may_open(), and renaming the hook to integrity_nameidata_check().
> > The nameidata is needed in order to open and read the file, so
> > that the file can be hashed(a cryptographically strong checksum.)
> > 
> > This patch also fixes the template locking, preventing the template
> > from being freed while being used.
> > 
> > This patch is a redesign of the integrity framework, which address a
> > number of issues, including
> >    - generalizing the measurement API beyond just inode measurements.
> >    - separation of the measurement into distinct collection, appraisal,
> >      and commitment phases, for greater flexibility.
> > 
> > Extended Verification Module(EVM) and the Integrity Measurement
> > Architecture(IMA) were originally implemented as an LSM module.  Based
> > on discussions on the LSM mailing list, a decision was made that the
> > LSM hooks should only be used to enforce mandatory access control
> > decisions and a new set of hooks should be defined specifically for
> > integrity.
> > 
> > EVM/IMA was limited to verifying and measuring a file's (i.e. an inode)
> > integrity and the metadata associated with it.  Current research is
> > looking into other types of integrity measurements. (i.e. "Linux kernel
> > integrity measurement using contextual inspection",  by Peter A. Loscocco,
> > Perry W. Wilson, J. Aaron Pendergrass, C. Durward McDonell,
> > http://doi.acm.org/10.1145/1314354.1314362). As a result, a requirement
> > of the new integrity framework is support for different types of integrity
> > measurements.
> > This patch provides an integrity framework(api and hooks) and placement
> > of the integrity hooks in the appropriate places in the fs directory.
> > Collecting, appraising, and storing of file and other types of integrity
> > data is supported.  Multiple integrity templates, which implement the
> > integrity API, may register themselves.  For now, only a single integrity
> > provider can register itself for the integrity hooks. (Support for multiple
> > providers registering themselves for the integrity hooks would require
> > some form of stacking.)
> > 
> > The six integrity hooks are:
> >    nameidata_check_integrity, inode_alloc_integrity, inode_free_integrity,
> >    bprm_check_integrity, file_free_integrity, file_mmap
> > 
> > The five integrity API calls provided are:
> >    integrity_must_measure, integrity_collect_measurement,
> >    integrity_appraise_measurement, integrity_store_measurement,
> >    and integrity_display_template.
> > 
> > The type of integrity data being collected, appraised, stored, or
> > displayed is template dependent.
> > 
> >
> > ...
> >
> > +int integrity_register_template(const char *template_name,
> > +				const struct template_operations *template_ops)
> > +{
> > +	int template_len;
> > +	struct template_list_entry *entry;
> > +
> > +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > +	if (!entry)
> > +		return -ENOMEM;
> > +	INIT_LIST_HEAD(&entry->template);
> > +
> > +	atomic_set(&entry->refcount, 1);
> > +	template_len = strlen(template_name);
> > +	if (template_len > TEMPLATE_NAME_LEN_MAX) {
> 
> It would be much neater to perform this check before running kzalloc().

Yes, will be moved in next patch set.

> > +		kfree(entry);
> > +		return -EINVAL;
> > +	}
> > +	strcpy(entry->template_name, template_name);
> > +	entry->template_ops = template_ops;
> > +
> > +	mutex_lock(&integrity_templates_mutex);
> > +	list_add_rcu(&entry->template, &integrity_templates);
> > +	mutex_unlock(&integrity_templates_mutex);
> > +	synchronize_rcu();
> > +
> > +	return 0;
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(integrity_register_template);
> 
> someone forgot to run checkpatch.

There's a couple of things like this where Lindent "fixes", and then
checkpatch complains.  In this case though, Lindent has been fixed. :-)

> >
> > ...
> >
> > +static inline void tget(struct template_list_entry *entry)
> > +{
> > +	if (!entry)
> > +		return;
> > +	atomic_inc(&entry->refcount);
> > +}
> > +
> > +static inline void tput(struct template_list_entry *entry)
> > +{
> > +	if (!entry)
> > +		return;
> > +	if (atomic_dec_and_test(&entry->refcount))
> > +		kfree(entry);
> > +}
> 
> Do these _really_ need to test for a NULL pointer?  It's an extra
> test-n-branch in many fastpaths.  It would be better to avoid doing
> this here, if poss.

Cleaned up the callers to avoid requiring the extra test.

Mimi