Messages in this thread Patch in this message | | | Date | Wed, 28 May 2008 00:46:10 -0700 | From | Andrew Morton <> | Subject | Re: [RFC][PATCH 4/5]integrity: Linux Integrity Module(LIM) |
| |
On Fri, 23 May 2008 11:05:33 -0400 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> This is a request for comments for a redesign of the integrity patches. > > ... > > +int register_template(char *template_name, > + struct template_operations *template_ops) > +{ > + int template_len; > + struct template_list_entry *entry; > + > + if (!template_initialized++) { > + INIT_LIST_HEAD(&integrity_templates); > + mutex_init(&integrity_templates_mutex); > + } > + > + entry = kmalloc(sizeof(*entry), GFP_ATOMIC); > + INIT_LIST_HEAD(&entry->template); > + > + template_len = strlen(template_name); > + if (template_len > TEMPLATE_NAME_LEN_MAX) > + template_len = TEMPLATE_NAME_LEN_MAX; > + memcpy(entry->template_name, template_name, template_len); > + entry->template_name[template_len] = '\0'; > + 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(register_template);
Should be integrity_register_template()?
> +/** > + * unregister_template > + * @template_name: a pointer to a string containing the template name. > + * > + * Unregister the template functions > + */ > +int unregister_template(char *template_name) > +{ > + struct template_list_entry *entry; > + > + mutex_lock(&integrity_templates_mutex); > + list_for_each_entry(entry, &integrity_templates, template) { > + if (strncmp(entry->template_name, template_name, > + strlen(entry->template_name)) == 0) { > + list_del_rcu(&entry->template); > + mutex_unlock(&integrity_templates_mutex); > + synchronize_rcu(); > + kfree(entry); > + return 0; > + } > + } > + mutex_unlock(&integrity_templates_mutex); > + return -EINVAL; > +} > +EXPORT_SYMBOL_GPL(unregister_template);
Similarly.
> +/** > + * integrity_find_template > + * @template_name: a pointer to a string containing the template name. > + * @template_ops: a pointer to the template functions > + * > + * Find the template functions based on the template name. > + */ > +int integrity_find_template(char *template_name, > + struct template_operations **template_ops) > +{ > + struct template_list_entry *entry; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &integrity_templates, template) { > + if (strncmp(entry->template_name, template_name, > + strlen(entry->template_name)) == 0) { > + *template_ops = entry->template_ops; > + rcu_read_unlock(); > + return 0; > + } > + } > + rcu_read_unlock(); > + return 1; > +} > +EXPORT_SYMBOL_GPL(integrity_find_template);
Strange locking. We take the rcu_read_lock then locate a pointer to an object then drop the lock, returning that pointer while doing nothing to ensure the stability of the returned object?
> > ... > > +#define set_to_dummy_if_null(ops, function) \ > + do { \ > + if (!ops->function) { \ > + ops->function = dummy_##function; \ > + printk(KERN_INFO "Had to override the " #function \ > + " integrity operation with the dummy one.\n");\ > + } \ > + } while (0)
hrm.
Probably the message should identify where it came from? "integrity: had to override ..."
> > ... > > @@ -1036,6 +1038,7 @@ struct dentry *d_instantiate_unique(stru > spin_unlock(&dcache_lock); > > if (!result) { > + integrity_d_instantiate(entry, inode); > security_d_instantiate(entry, inode); > return NULL; > } > @@ -1173,6 +1176,7 @@ struct dentry *d_splice_alias(struct ino > BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED)); > fsnotify_d_instantiate(new, inode); > spin_unlock(&dcache_lock); > + integrity_d_instantiate(new, inode); > security_d_instantiate(new, inode); > d_rehash(dentry); > d_move(new, dentry); > @@ -1183,6 +1187,7 @@ struct dentry *d_splice_alias(struct ino > dentry->d_inode = inode; > fsnotify_d_instantiate(dentry, inode); > spin_unlock(&dcache_lock); > + integrity_d_instantiate(dentry, inode); > security_d_instantiate(dentry, inode); > d_rehash(dentry); > } > @@ -1733,6 +1738,7 @@ found: > spin_unlock(&dcache_lock); > out_nolock: > if (actual == dentry) { > + integrity_d_instantiate(dentry, inode); > security_d_instantiate(dentry, inode); > return NULL; > }
I'm trying to find a non-trivial ->d_instantiate() implementation to see how much overhead is being added to these performance-critical codepaths, but afaict this patchset doesn't add one?
> Index: linux-2.6.26-rc3-git2/fs/ext3/xattr_security.c > =================================================================== > --- linux-2.6.26-rc3-git2.orig/fs/ext3/xattr_security.c > +++ linux-2.6.26-rc3-git2/fs/ext3/xattr_security.c > @@ -9,6 +9,7 @@ > #include <linux/ext3_jbd.h> > #include <linux/ext3_fs.h> > #include <linux/security.h> > +#include <linux/integrity.h> > #include "xattr.h" > > static size_t > @@ -57,12 +58,19 @@ ext3_init_security(handle_t *handle, str > > err = security_inode_init_security(inode, dir, &name, &value, &len); > if (err) { > + /* Even if creation of the security xattr fails, must > + * indicate this is a new inode. */ > + integrity_inode_init_integrity(inode, dir, NULL, NULL, NULL); > if (err == -EOPNOTSUPP) > return 0; > return err; > } > err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY, > name, value, len, 0); > + > + integrity_inode_init_integrity(inode, dir, &name, &value, &len); > + err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY, > + name, value, len, 0);
Can we put the integrity_inode_init_integrity() call into security_inode_init_security() thus avoiding having to change lots of filesystems?
> kfree(name); > kfree(value); > return err; > > ... > > @@ -17,6 +17,7 @@ > #include <linux/hash.h> > #include <linux/swap.h> > #include <linux/security.h> > +#include <linux/integrity.h> > #include <linux/pagemap.h> > #include <linux/cdev.h> > #include <linux/bootmem.h> > @@ -160,6 +161,14 @@ static struct inode *alloc_inode(struct > init_rwsem(&inode->i_alloc_sem); > lockdep_set_class(&inode->i_alloc_sem, &sb->s_type->i_alloc_sem_key); > > + if (integrity_inode_alloc(inode)) { > + if (inode->i_sb->s_op->destroy_inode) > + inode->i_sb->s_op->destroy_inode(inode); > + else > + kmem_cache_free(inode_cachep, (inode)); > + return NULL; > + }
This code is uncommented and integrity_inode_alloc() also is uncommented. People will want to know what's going on, please.
Again, where do we go to see how much overhead is being added to these codepaths?
> mapping->a_ops = &empty_aops; > mapping->host = inode; > mapping->flags = 0; > @@ -190,6 +199,7 @@ void destroy_inode(struct inode *inode) > { > BUG_ON(inode_has_buffers(inode)); > security_inode_free(inode); > + integrity_inode_free(inode); > if (inode->i_sb->s_op->destroy_inode) > inode->i_sb->s_op->destroy_inode(inode); > else > > ... > > --- linux-2.6.26-rc3-git2.orig/include/linux/fs.h > +++ linux-2.6.26-rc3-git2/include/linux/fs.h > @@ -653,6 +653,9 @@ struct inode { > #ifdef CONFIG_SECURITY > void *i_security; > #endif > +#ifdef CONFIG_INTEGRITY > + void *i_integrity; > +#endif
Does this _have_ to be a void*? Something typesafe would be far preferable.
> void *i_private; /* fs or device private pointer */ > }; > > ... > > @@ -521,6 +528,7 @@ extern int audit_signals; > #define audit_get_loginuid(t) (-1) > #define audit_get_sessionid(t) (-1) > #define audit_log_task_context(b) do { ; } while (0) > +#define audit_log_inode_context(b, a) do { ; } while (0)
static inline C functions are preferable.
The ";" inside the {} is unneeded.
> #define audit_ipc_obj(i) ({ 0; }) > #define audit_ipc_set_perm(q,u,g,m) ({ 0; }) > #define audit_bprm(p) ({ 0; }) > Index: linux-2.6.26-rc3-git2/security/integrity/integrity_audit.c > =================================================================== > --- /dev/null > +++ linux-2.6.26-rc3-git2/security/integrity/integrity_audit.c > @@ -0,0 +1,73 @@ > +/* > + * Copyright (C) 2008 IBM Corporation > + * Author: Mimi Zohar <zohar@us.ibm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, version 2 of the License. > + * > + * File: integrity_audit.c > + * Audit calls for the integrity subsystem > + */ > + > +#include <linux/audit.h> > +#include <linux/fs.h> > +#include <linux/integrity.h> > + > +#ifdef CONFIG_INTEGRITY_AUDIT > +static int integrity_audit = 1; > + > +static int __init integrity_audit_setup(char *str) > +{ > + char *op; > + > + integrity_audit = simple_strtol(str, NULL, 0); This will treat "42foo" as valid input. strict_strtoul() fixes that.
> + op = integrity_audit ? "integrity_audit_enabled" : > + "integrity_audit_not_enabled"; > + integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, NULL, op, 0); > + return 1; > +} > + > +__setup("integrity_audit=", integrity_audit_setup); > +#else > +static int integrity_audit = 1;
This could be outside the ifdef. If it is correct?
> +#endif > + > +void integrity_audit_msg(int audit_msgno, struct inode *inode, > + const unsigned char *fname, char *op, > + char *cause, int result) > +{ > + struct audit_buffer *ab; > + if (!integrity_audit && result == 1) > + return;
A newline between end-of-declarations and start-of-code is conventional.
> + ab = audit_log_start(current->audit_context, GFP_ATOMIC, audit_msgno); > + audit_log_format(ab, "integrity: pid=%d uid=%u auid=%u", > + current->pid, current->uid, > + audit_get_loginuid(current)); > + audit_log_task_context(ab); > + switch (audit_msgno) { > + case AUDIT_INTEGRITY_DATA: > + case AUDIT_INTEGRITY_METADATA: > + case AUDIT_INTEGRITY_PCR: > + audit_log_format(ab, " op=%s cause=%s", op, cause); > + break; > + case AUDIT_INTEGRITY_HASH: > + audit_log_format(ab, " op=%s hash=%s", op, cause); > + break; > + case AUDIT_INTEGRITY_STATUS: > + default: > + audit_log_format(ab, " op=%s", op); > + } > + audit_log_format(ab, " comm="); > + audit_log_untrustedstring(ab, current->comm); > + if (fname) { > + audit_log_format(ab, " name="); > + audit_log_untrustedstring(ab, fname); > + } > + if (inode) > + audit_log_format(ab, " dev=%s ino=%lu", > + inode->i_sb->s_id, inode->i_ino); > + audit_log_format(ab, " res=%d", result); > + audit_log_end(ab); > +} > > ... > > +static struct integrity_measure_rule_entry default_rules[] = { > + {{NULL, NULL}, NULL, NULL, FILE_MMAP, MAY_EXEC}, > + {{NULL, NULL}, NULL, NULL, BPRM_CHECK, MAY_EXEC}, > + {{NULL, NULL}, NULL, NULL, INODE_PERMISSION, MAY_READ}, > +};
Can we use the
.field = value,
format here please? That will permit the omission of all the NULLs.
> +static struct list_head measure_default_rules; > +static struct list_head measure_policy_rules; > +static struct list_head *integrity_measure; > + > +static DEFINE_MUTEX(integrity_measure_mutex); > + > +/** > + * integrity_measure_rules - determine whether an inode matches the given rule. > + * @rule - a pointer to a rule > + * @inode - a pointer to an inode > + * @func - LIM hook identifier > + * @mask - requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC) > + * > + * Returns 1 on rule match, 0 on failure. > + */
What a strange name. integrity_match_rules()?
Could return a bool type, if you like that sort of thing.
> +static int integrity_measure_rules(struct integrity_measure_rule_entry *rule, > + struct inode *inode, enum lim_hooks func, > + int mask) > +{ > + int result = 1; > + > + if (result && (rule->func != 0)) {
The test of the known-to-be-non-zero `result' is a bit weird, btu I guess it makes sense in context, and the compiler should elide it OK.
> + if (rule->func != func) > + result = 0; > + } > + if (result && (rule->mask != 0)) { > + if (rule->mask != mask) > + result = 0; > + } > + if (result && rule->lsm_subj_rule) { > + struct task_struct *tsk = current; > + u32 sid; > + > + security_task_getsecid(tsk, &sid); > + result = security_filter_rule_match(sid, AUDIT_SUBJ_USER, > + AUDIT_EQUAL, > + rule->lsm_subj_rule, NULL); > + } > + if (result && rule->lsm_obj_rule) { > + u32 osid; > + > + security_inode_getsecid(inode, &osid); > + result = security_filter_rule_match(osid, AUDIT_OBJ_USER, > + AUDIT_EQUAL, > + rule->lsm_obj_rule, NULL); > + } > + return result; > +}
However the shole function could be simplified and sped up (depending on how smart the compiler is) via:
if (rule->func && rule->func != func) return 0; if (rule->mask && rule->mask != mask) return 0; ... return 1; } or similar.
> +/** > + * integrity_measure_policy - base measure decision on: subj, obj, LIM hook, > + * and mask > + * @inode - pointer to an inode > + * @func - LIM hook identifier > + * @mask - requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC) > + * > + * Returns 1 on rule match, 0 on failure. > + */ > +int integrity_measure_policy(struct inode *inode, enum lim_hooks func, int mask) > +{ > + struct integrity_measure_rule_entry *entry; > + int rc = 0; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, integrity_measure, list) { > + rc = integrity_measure_rules(entry, inode, func, mask); > + if (rc) { > + rcu_read_unlock(); > + return rc; > + } > + } > + rcu_read_unlock(); > + return rc; > +}
"measure"? Or "match"?
> +/** > + * integrity_measure_policy_init - initialize the default and policy rules. > + */ > +void integrity_measure_policy_init(void) > +{ > + int i; > + > + INIT_LIST_HEAD(&measure_default_rules); > + for (i = 0; i < ARRAY_SIZE(default_rules); i++) > + list_add(&default_rules[i].list, &measure_default_rules); > + integrity_measure = &measure_default_rules; > + mutex_init(&integrity_measure_mutex);
The mutex_init() is unneeded.
> + INIT_LIST_HEAD(&measure_policy_rules); > +} > + > +/** > + * integrity_measure_policy_complete - wait to replace default_rules with > + * a complete policy ruleset. > + */ > +void integrity_measure_policy_complete(void) > +{ > + char *op = "policy_update"; > + char *cause = "already exists"; > + int result = 1; > + > + if (integrity_measure == &measure_default_rules) { > + integrity_measure = &measure_policy_rules; > + cause = "complete"; > + result = 0; > + } > + integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, > + NULL, op, cause, result); > +}
Does it actually "wait"?
The name again doesn't seem to match the behaviour. "foo_complete" would mean "tell listeners that foo has completed". What you have here is a "foo_wait".
> +/** > + * integrity_measure_rule_add - add integrity measure rules > + * @subj - pointer to an LSM subject value > + * @obj - pointer to an LSM object value > + * @func - LIM hook identifier > + * @mask - requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC) > + * > + * Returns 0 on success, an error code on failure. > + */ > +int integrity_measure_rule_add(char *subj, char *obj, char *func, char *mask) > +{ > + struct integrity_measure_rule_entry *entry; > + int result = 0; > + > + /* Prevent installed policy from changing */ > + if (integrity_measure != &measure_default_rules) { > + integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, > + NULL, "policy_update", "already exists", 1); > + return -EACCES; > + } > + > + entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
GFP_ATOMIC is unreliable. GFP_KERNEL is much much preferable, and I suspect that it can be used here?
> + INIT_LIST_HEAD(&entry->list); > + if (!result && subj) > + result = security_filter_rule_init(AUDIT_SUBJ_USER, AUDIT_EQUAL, > + subj, &entry->lsm_subj_rule); > + if (!result && obj) > + result = security_filter_rule_init(AUDIT_OBJ_USER, AUDIT_EQUAL, > + obj, &entry->lsm_obj_rule); > + if (!result && func) { > + if (strcmp(func, "INODE_PERMISSION") == 0) > + entry->func = INODE_PERMISSION; > + else if (strcmp(func, "FILE_MMAP") == 0) > + entry->func = FILE_MMAP; > + else if (strcmp(func, "BPRM_CHECK") == 0) > + entry->func = BPRM_CHECK; > + else > + result = -EINVAL; > + } > + if (!result && mask) { > + if (strcmp(mask, "MAY_EXEC") == 0) > + entry->mask = MAY_EXEC; > + else if (strcmp(mask, "MAY_WRITE") == 0) > + entry->mask = MAY_WRITE; > + else if (strcmp(mask, "MAY_READ") == 0) > + entry->mask = MAY_READ; > + else if (strcmp(mask, "MAY_APPEND") == 0) > + entry->mask = MAY_APPEND; > + else > + result = -EINVAL; > + } > + if (!result) { > + mutex_lock(&integrity_measure_mutex); > + list_add_tail(&entry->list, &measure_policy_rules); > + mutex_unlock(&integrity_measure_mutex); > + } > + return result; > +} > > ...
| |