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

[Andrew Morton <akpm@linux-foundation.org>]

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().

> +		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.

>
> ...
>
> +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.