[PATCH] support for context based audit filtering

[Darrel Goeddel]

Amy Griffis wrote:
> 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
 <snip>
>>@@ -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.

I originally went with that approach, but later decided to break it out
to make a generic copy function (which I scrapped in favor of the
specialized helper because I didn't know anything about the watches...).
I'm assuming that this whole thing will need modification based on your
continued fs auditing work.  I'll keep this in mind for the next version.

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

Yep.  Thanks.

> 
>>+			/* 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?

As I mentioned before, I'm not really sure on the whole watch situation.
Sounds right though.  se_str will need to be copied and se_rule would need
to be re-initialized using selinux_audit_rule_init() because it is an
opaque item.

> 
>>+	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) ?

Yep.

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

Yep.  Should be tmperr.

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

That can be done.  I liked keeping this all scoped together.  We could put
this part in audit_init (audit.c) and put the declaration for
selinux_audit_rule_update in kernel/audit.h.

Thanks for the feedback.  I'm sure this stuff will look much better when
integrated with your audit fs work.

-- 

Darrel