Messages in this thread | | | Subject | Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM) | From | Mimi Zohar <> | Date | Mon, 17 Nov 2008 14:04:41 -0500 |
| |
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
| |