[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] support for context based audit filtering



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]