Re: [PATCH] support for context based audit filtering

Hi Darrel, I didn't notice this patch in my inbox until just recently. I've put a few comments inline. On Fri, Feb 24, 2006 at 04:26:13PM -0600, Darrel Goeddel wrote: >diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >index 3712295..752e2bb 100644 >--- a/kernel/auditfilter.c >+++ b/kernel/auditfilter.c

>@@ -726,3 +784,143 @@ unlock_and_return: > rcu_read_unlock(); > return result; >} >+ >+/* Check to see if the rule contains any selinux fields. Returns 1 if >there >+ are selinux fields specified in the rule, 0 otherwise. */ >+static inline int audit_rule_has_selinux(struct audit_krule *rule) >+{ >+ int i; >+ >+ for (i = 0; i < rule->field_count; i++) { >+ struct audit_field *f = &rule->fields[i]; >+ switch (f->type) { >+ case AUDIT_SE_USER: >+ case AUDIT_SE_ROLE: >+ case AUDIT_SE_TYPE: >+ case AUDIT_SE_SEN: >+ case AUDIT_SE_CLR: >+ return 1; >+ } >+ } >+ >+ return 0; >+} >+ >+/* Make a copy of src in dest. This will be a deep copy with the exception >+ of the watch - that pointer is carried over. The selinux specific >fields >+ will be updated in the copy. The point is to be able to replace the src >+ rule with the dest rule in the list, then free the dest rule. */ >+static inline int selinux_audit_rule_update_helper(struct audit_krule >*dest, >+ struct audit_krule *src) >+{ >+ int i, err = 0; >+ >+ dest->vers_ops = src->vers_ops; >+ dest->flags = src->flags; >+ dest->listnr = src->listnr; >+ dest->action = src->action; >+ for (i = 0; i < AUDIT_BITMASK_SIZE; i++) >+ dest->mask[i] = src->mask[i]; >+ dest->buflen = src->buflen; >+ dest->field_count = src->field_count; >+ >+ /* deep copy this information, updating the se_rule fields, because >+ the originals will all be freed when the old rule is freed. */ >+ dest->fields = kzalloc(sizeof(struct audit_field) * >dest->field_count, >+ GFP_ATOMIC); >+ if (!dest->fields) >+ return -ENOMEM; It seems like it would be cleaner to group the memory allocations for the rule entry and the fields into a single function, as any audit rule must always have both. This would allow you to retain the assumption of the existence of fields in audit_free_rule.

>+ memcpy(dest->fields, src->fields, >+ sizeof(struct audit_field) * dest->field_count); >+ for (i = 0; i < dest->field_count; i++) { >+ struct audit_field *df = &dest->fields[i]; >+ struct audit_field *sf = &src->fields[i]; >+ switch (df->type) { >+ case AUDIT_SE_USER: >+ case AUDIT_SE_ROLE: >+ case AUDIT_SE_TYPE: >+ case AUDIT_SE_SEN: >+ case AUDIT_SE_CLR: >+ /* our own copy of se_str */ >+ df->se_str = kstrdup(sf->se_str, GFP_ATOMIC); I realize failure is unlikely here, but shouldn't you check the return value? Later on you'll end up passing this to strcmp() which won't like it if it's NULL.

>+ /* our own (refreshed) copy of se_rule */ >+ err = selinux_audit_rule_init(df->type, df->op, >+ df->se_str, >&df->se_rule); >+ /* Keep currently invalid fields around in case they >+ become valid after a policy reload. */ >+ if (err == -EINVAL) { >+ printk(KERN_WARNING "selinux audit rule for >item %s is invalid\n", df->se_str); >+ err = 0; >+ } >+ if (err) >+ return err; >+ } >+ } >+ >+ /* we can shallow copy the watch because we will not be freeing it >via >+ selinux_audit_rule_update (and we do nto modify it) */ On the flipside, when audit watches are updated, the se_str and se_rule will need to be deep copied since they are freed in audit_free_rule(). Is this what you intend?

>+ dest->watch = src->watch; >+ dest->rlist = src->rlist; >+ >+ return 0; >+} >+ >+/* This function will re-initialize the se_rule field of all applicable >rules. >+ It will traverse the filter lists serarching for rules that contain >selinux >+ specific filter fields. When such a rule is found, it is copied, the >+ selinux field is re-initialized, and the old rule is replaced with the >+ updated rule. */ >+/* XXX: is the error handling below appropriate */ >+static int selinux_audit_rule_update(void) >+{ >+ struct audit_entry *entry, *nentry; >+ int i, err = 0, tmperr; >+ >+ /* audit_netlink_sem synchronizes the writers */ >+ down(&audit_netlink_sem); >+ >+ for (i = 0; i < AUDIT_NR_FILTERS; i++) { >+ list_for_each_entry(entry, &audit_filter_list[i], list) { >+ if (!audit_rule_has_selinux(&entry->rule)) >+ continue; >+ >+ nentry = kmalloc(sizeof(*entry), GFP_ATOMIC); >+ if (!nentry) { >+ /* save the first error encountered for the >+ return value */ >+ if (!err) >+ err = -ENOMEM; >+ audit_panic("error updating selinux >filters"); >+ continue; >+ } >+ >+ tmperr = >selinux_audit_rule_update_helper(&nentry->rule, >+ >&entry->rule); >+ if (!nentry) { How would we end up with !nentry here? Maybe you mean if (temperr) ?

>+ /* save the first error encountered for the >+ return value */ >+ if (!err) >+ err = -ENOMEM; You don't want to hardcode the -ENOMEM as selinux_audit_rule_init() can also return an error.

>+ audit_free_rule(nentry); >+ audit_panic("error updating selinux >filters"); >+ continue; >+ } >+ list_replace_rcu(&entry->list, &nentry->list); >+ call_rcu(&entry->rcu, audit_free_rule_rcu); >+ } >+ } >+ >+ up(&audit_netlink_sem); >+ >+ return err; >+} >+ >+/* Register the callback with selinux. This callback will be invoked when >a >+ new policy is loaded. */ >+static int __init register_selinux_update_callback(void) >+{ >+ selinux_audit_set_callback(&selinux_audit_rule_update); >+ return 0; >+} >+__initcall(register_selinux_update_callback); >+ I don't know about anyone else, but I would prefer to keep all of the initialization for the audit subsystem in audit.c:audit_init(). This makes the audit initialization path more easily synchronized and readable.