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

[WIP][PATCH] Audit inotify client



Hello,

This patch allows audit to operate as an inotify client.

It adds a list of parents, which represent the dentry parents of the
filesystem locations to be watched.  When created, a parent registers
an inotify watch on itself.  If all the audit rules corresponding to a
parent are removed by the admin, the parent removes its inotify watch
before it is destroyed.

Audit's inotify callback, audit_handle_fsevent(), is called following
a specified group of inotify events.  These events translate to one of
two activities for audit.  It may update rules in the syscall exit
filterlist with a new inode number or the value -1, or it may
implicitly remove all watches and rules associated with a particular
parent (that has been removed from the filesystem).

The patch is based off of Al Viro's audit git tree, applying after
these previously posted patches:

audit rule interface changes:
http://www.redhat.com/archives/linux-audit/2006-January/msg00064.html
http://www.redhat.com/archives/linux-audit/2006-January/msg00082.html

inotify kernel api:
https://www.redhat.com/archives/linux-audit/2006-January/msg00084.html

This patch is still a work in progress.  There are a couple of race
conditions that need to be properly handled, as well as some
additional synchronization needed for concurrent manipulations of the
filterlist (list_del_rcu, list_update_rcu, etc.).  For this I'm
planning to add a per-element spinlock to be taken for list
manipulations only.

Before finishing up these last pieces, I wanted to post my current
work for comments on the locking approach.  Please have a look and let
me know what you think.

Thanks,
Amy


diff --git a/kernel/audit.c b/kernel/audit.c
index bdda766..e8b6b8f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -55,6 +55,9 @@
 #include <net/netlink.h>
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
+#include <linux/inotify.h>
+
+#include "audit.h"
 
 /* No auditing will take place until audit_initialized != 0.
  * (Initialization happens after skb_init is called.) */
@@ -99,6 +102,9 @@ static atomic_t    audit_lost = ATOMIC_I
 /* The netlink socket. */
 static struct sock *audit_sock;
 
+/* Inotify device. */
+struct inotify_device *audit_idev;
+
 /* The audit_freelist is a list of pre-allocated audit buffers (if more
  * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
  * being placed on the freelist). */
@@ -564,6 +570,11 @@ static int __init audit_init(void)
 	audit_initialized = 1;
 	audit_enabled = audit_default;
 	audit_log(NULL, GFP_KERNEL, AUDIT_KERNEL, "initialized");
+
+	audit_idev = inotify_init(audit_handle_fsevent);
+	if (IS_ERR(audit_idev))
+		audit_panic("cannot initialize inotify device");
+
 	return 0;
 }
 __initcall(audit_init);
diff --git a/kernel/audit.h b/kernel/audit.h
index 5033e1f..e6a3135 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -22,6 +22,8 @@
 #include <linux/fs.h>
 #include <linux/audit.h>
 
+struct inotify_event;
+
 /* 0 = no checking
    1 = put_count checking
    2 = verbose put_count checking
@@ -52,10 +54,22 @@ enum audit_state {
 };
 
 /* Rule lists */
+struct audit_parent {
+	atomic_t		count;	 /* reference count */
+	unsigned long		ino;	 /* associated inode number */
+	u32			wd;	 /* inotify watch descriptor */
+	struct list_head	mlist;	 /* entry in master_parents */
+	struct list_head	watches; /* associated watches */
+	struct semaphore	watches_sem; /* protects parent's watches list*/
+};
+
 struct audit_watch {
+	atomic_t		count;	 /* reference count */
 	char			*path;	 /* watch insertion path */
-	struct list_head	mlist;	 /* entry in master_watchlist */
 	struct list_head	rules;	 /* associated rules */
+	struct semaphore	rules_sem; /* protects watch's rules list */
+	struct list_head	wlist;	 /* entry in audit_parent.watches list*/
+	struct audit_parent	*parent; /* associated parent */
 };
 
 struct audit_field {
@@ -86,7 +100,9 @@ struct audit_entry {
 
 extern int audit_pid;
 extern int audit_comparator(const u32 left, const u32 op, const u32 right);
-
+extern void audit_handle_fsevent(struct inotify_event *event,
+				 const char *name, struct inode * inode,
+				 void *ptr);
 extern void		    audit_send_reply(int pid, int seq, int type,
 					     int done, int multi,
 					     void *payload, int size);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 6506084..dbe0c98 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -25,6 +25,7 @@
 #include <linux/fs.h>
 #include <linux/namei.h>
 #include <linux/netlink.h>
+#include <linux/inotify.h>
 #include "audit.h"
 
 /* There are three lists of rules -- one to search at task creation
@@ -42,7 +43,84 @@ struct list_head audit_filter_list[AUDIT
 #endif
 };
 
-static LIST_HEAD(master_watchlist);
+static LIST_HEAD(master_parents);
+static DEFINE_SPINLOCK(master_parents_lock);
+
+/* Inotify device. */
+extern struct inotify_device *audit_idev;
+
+/* Inotify events we care about. */
+#define AUDIT_FSEVENTS	IN_MOVE|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF
+
+static inline void audit_get_parent(struct audit_parent *parent)
+{
+	atomic_inc(&parent->count);
+}
+
+static inline void audit_put_parent(struct audit_parent *parent)
+{
+	if (atomic_dec_and_test(&parent->count))
+		kfree(parent);
+}
+
+static inline void audit_get_watch(struct audit_watch *watch)
+{
+	atomic_inc(&watch->count);
+}
+
+static inline void audit_put_watch(struct audit_watch *watch)
+{
+	if (atomic_dec_and_test(&watch->count)) {
+		if (watch->parent)
+			audit_put_parent(watch->parent);
+		kfree(watch->path);
+		kfree(watch);
+	}
+}
+
+/* Initialize a parent watch entry.  Caller must pin inode (via nameidata). */
+static inline struct audit_parent *audit_init_parent(struct inode *inode)
+{
+	struct audit_parent *parent;
+	u32 wd;
+
+	parent = kmalloc(sizeof(*parent), GFP_KERNEL);
+	if (unlikely(!parent))
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&parent->watches);
+	init_MUTEX(&parent->watches_sem);
+	atomic_set(&parent->count, 0);
+	parent->ino = inode->i_ino;
+	audit_get_parent(parent);
+
+	wd = inotify_add_watch(audit_idev, inode, AUDIT_FSEVENTS, parent);
+	if (wd < 0) {
+		audit_put_parent(parent);
+		return ERR_PTR(wd);
+	}
+	parent->wd = wd;
+
+	return parent;
+}
+
+/* Initialize a watch entry. */
+static inline struct audit_watch *audit_init_watch(char *path)
+{
+	struct audit_watch *watch;
+
+	watch = kmalloc(sizeof(*watch), GFP_KERNEL);
+	if (unlikely(!watch))
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&watch->rules);
+	init_MUTEX(&watch->rules_sem);
+	atomic_set(&watch->count, 0);
+	watch->path = path;
+	audit_get_watch(watch);
+
+	return watch;
+}
 
 /* Unpack a filter field's string representation from user-space
  * buffer. */
@@ -75,7 +153,6 @@ static char *audit_unpack_string(void **
 static int audit_to_watch(char *path, struct audit_krule *krule, int fidx)
 {
 	struct audit_field *f = &krule->fields[fidx];
-	struct nameidata nd;
 	struct audit_watch *watch;
 
 	if (path[0] != '/' || path[f->val-1] == '/' ||
@@ -83,17 +160,13 @@ static int audit_to_watch(char *path, st
 	    f->op & ~AUDIT_EQUAL)
 		return -EINVAL;
 
-	if (path_lookup(path, 0, &nd) == 0)
-		f->val = nd.dentry->d_inode->i_ino;
-	else
-		f->val = (unsigned int)-1;
-	path_release(&nd);
+	watch = audit_init_watch(path);
+	if (unlikely(IS_ERR(watch)))
+		return PTR_ERR(watch);
 
-	watch = kmalloc(sizeof(*watch), GFP_KERNEL);
-	if (unlikely(!watch))
-		return -ENOMEM;
-	watch->path = path;
+	audit_get_watch(watch);
 	krule->watch = watch;
+	f->val = (unsigned int)-1;
 
 	return 0;
 }
@@ -325,7 +398,7 @@ static inline int audit_compare_watch(st
  * don't match. */
 static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
 {
-	int i;
+	int i, nomatch;
 
 	if (a->flags != b->flags ||
 	    a->listnr != b->listnr ||
@@ -340,7 +413,8 @@ static int audit_compare_rule(struct aud
 
 		switch(a->fields[i].type) {
 		case AUDIT_WATCH:
-			if (audit_compare_watch(a->watch, b->watch))
+			nomatch = audit_compare_watch(a->watch, b->watch);
+			if (nomatch)
 				return 1;
 			break;
 		default:
@@ -356,36 +430,184 @@ static int audit_compare_rule(struct aud
 	return 0;
 }
 
-static inline void audit_free_watch(struct audit_watch *watch)
+static inline void audit_free_rule(struct audit_entry *e)
 {
-	kfree(watch->path);
-	kfree(watch);
+	if (e->rule.watch)
+		audit_put_watch(e->rule.watch);
+	kfree(e);
 }
 
-static inline void audit_free_rule(struct rcu_head *head)
+static inline void audit_free_rule_rcu(struct rcu_head *head)
 {
 	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
-	kfree(e);
+	audit_free_rule(e);
+}
+
+static void audit_update_field(struct audit_krule *krule, u32 type, u32 val)
+{
+	int i;
+	struct audit_entry *old_e, *entry;
+
+	for (i = 0; i < AUDIT_MAX_FIELDS; i++)
+		if (krule->fields[i].type == type) {
+			entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+			if (unlikely(!entry))
+				return; /* XXX */
+
+			old_e = container_of(krule, struct audit_entry, rule);
+			memcpy(entry, old_e, sizeof(struct audit_entry));
+			entry->rule.fields[i].val = val;
+
+			list_replace_rcu(&old_e->list, &entry->list);
+			call_rcu(&old_e->rcu, audit_free_rule_rcu);
+			return;
+		}
+}
+
+static inline void audit_handle_update(struct audit_parent *parent,
+				       const char *name, u32 ino)
+{
+	struct audit_watch *w;
+	struct audit_krule *r;
+
+	audit_get_parent(parent);
+	down(&parent->watches_sem);
+	list_for_each_entry(w, &parent->watches, wlist) {
+		audit_get_watch(w);
+		if (strcmp(w->path, name)) { /* XXX */
+			audit_put_watch(w);
+			continue;
+		}
+
+		down(&w->rules_sem);
+		list_for_each_entry(r, &w->rules, rlist)
+			audit_update_field(r, AUDIT_WATCH, ino);
+		up(&w->rules_sem);
+		audit_put_watch(w);
+	}
+	up(&parent->watches_sem);
+	audit_put_parent(parent);
 }
 
-/* Attach krule's watch to master_watchlist, using existing watches
- * when possible. */
-static inline void audit_add_watch(struct audit_krule *krule)
+static inline void audit_handle_removal(struct audit_parent *parent)
 {
 	struct audit_watch *w;
+	struct audit_krule *r;
+	struct audit_entry *e;
+
+	audit_get_parent(parent);
+	down(&parent->watches_sem);
+	list_for_each_entry(w, &parent->watches, wlist) {
+		audit_get_watch(w);
+		down(&w->rules_sem);
+		list_for_each_entry(r, &w->rules, rlist) {
+			e = container_of(r, struct audit_entry, rule);
+			list_del_rcu(&e->list);
+			call_rcu(&e->rcu, audit_free_rule_rcu);
+		}
+		up(&w->rules_sem);
+		audit_put_watch(w);
+	}
+	spin_lock(&master_parents_lock);
+	list_del(&parent->mlist);
+	spin_unlock(&master_parents_lock);
+	up(&parent->watches_sem);
+	audit_put_parent(parent);
+
+}
 
-	list_for_each_entry(w, &master_watchlist, mlist) {
-		if (audit_compare_watch(w, krule->watch))
+void audit_handle_fsevent(struct inotify_event *event, const char *name,
+			  struct inode *inode, void *ptr)
+{
+	struct audit_parent *parent = (struct audit_parent *)ptr;
+
+	if (event->mask & (IN_CREATE|IN_MOVED_TO) && inode)
+		audit_handle_update(parent, name, (unsigned int)inode->i_ino);
+	else if (event->mask & (IN_DELETE|IN_MOVED_FROM))
+		audit_handle_update(parent, name, (unsigned int)-1);
+	else if (event->mask & (IN_DELETE_SELF|IN_MOVE_SELF))
+		audit_handle_removal(parent);
+}
+
+/* Find an existing parent entry for this watch, or create a new one. */
+static inline struct audit_parent *audit_find_parent(struct inode *inode)
+{
+	struct audit_parent *p, *next, *parent;
+
+	list_for_each_entry_safe(p, next, &master_parents, mlist) {
+		if (p->ino != inode->i_ino)
 			continue;
 
-		audit_free_watch(krule->watch);
-		krule->watch = w;
-		list_add(&krule->rlist, &w->rules);
-		return;
-	}
-	INIT_LIST_HEAD(&krule->watch->rules);
-	list_add(&krule->rlist, &krule->watch->rules);
-	list_add(&krule->watch->mlist, &master_watchlist);
+		audit_get_parent(p); /* hold ref until we take watches_sem */
+		parent = p;
+		goto out;
+	}
+
+	parent = audit_init_parent(inode);
+	if (unlikely(IS_ERR(parent)))
+		goto out;
+
+	audit_get_parent(parent); /* hold ref until we take watches_sem */
+
+	spin_lock(&master_parents_lock);
+	list_add(&parent->mlist, &master_parents);
+	spin_unlock(&master_parents_lock);
+
+out:
+	return parent;
+}
+
+/* Find a matching watch entry, or add this one. */
+static inline int audit_add_watch(struct audit_krule *krule)
+{
+	struct nameidata nd;
+	struct audit_parent *parent;
+	struct audit_watch *w, *watch = krule->watch;
+	int ret = 0;
+
+	ret = path_lookup(watch->path, LOOKUP_PARENT, &nd);
+	if (ret)
+		goto out;
+
+	parent = audit_find_parent(nd.dentry->d_inode);
+	if (IS_ERR(parent)) {
+		ret = PTR_ERR(parent);
+		path_release(&nd);
+		goto out;
+	}
+	path_release(&nd);
+
+	down(&parent->watches_sem);
+	audit_put_parent(parent);
+	list_for_each_entry(w, &parent->watches, wlist) {
+		if (audit_compare_watch(watch, w))
+			continue;
+
+		audit_put_watch(watch); /* krule's ref */
+		audit_put_watch(watch); /* destroy */
+
+		audit_get_watch(w);
+		krule->watch = watch = w;
+		goto add_rule;
+	}
+
+	audit_get_parent(parent);
+	watch->parent = parent;
+	list_add(&watch->wlist, &parent->watches);
+
+add_rule:
+	down(&watch->rules_sem);
+	list_add(&krule->rlist, &watch->rules);
+	up(&watch->rules_sem);
+	up(&parent->watches_sem);
+
+	if (path_lookup(watch->path, 0, &nd) == 0)
+		audit_update_field(krule, AUDIT_WATCH,
+				   nd.dentry->d_inode->i_ino);
+	path_release(&nd);
+
+out:
+	return ret;
 }
 
 /* Add rule to given filterlist if not a duplicate.  Protected by
@@ -394,16 +616,25 @@ static inline int audit_add_rule(struct 
 				  struct list_head *list)
 {
 	struct audit_entry *e;
+	int err;
 
-	/* Do not use the _rcu iterator here, since this is the only
-	 * addition routine. */
-	list_for_each_entry(e, list, list) {
-		if (!audit_compare_rule(&entry->rule, &e->rule))
-			return -EEXIST;
+	/* The *_rcu iterator is needed to protect from filesystem
+	 * updates or removals. */
+	rcu_read_lock();
+	list_for_each_entry_rcu(e, list, list) {
+		if (!audit_compare_rule(&entry->rule, &e->rule)) {
+			rcu_read_unlock();
+			err = -EEXIST;
+			goto error;
+		}
 	}
+	rcu_read_unlock();
 
-	if (entry->rule.watch)
-		audit_add_watch(&entry->rule);
+	if (entry->rule.watch) {
+		err = audit_add_watch(&entry->rule);
+		if (err)
+			goto error;
+	}
 	if (entry->rule.flags & AUDIT_FILTER_PREPEND) {
 		list_add_rcu(&entry->list, list);
 	} else {
@@ -411,20 +642,48 @@ static inline int audit_add_rule(struct 
 	}
 
 	return 0;
+
+error:
+	if (entry->rule.watch)
+		audit_put_watch(entry->rule.watch);
+	return err;
 }
 
-/* Detach watch from krule, freeing if it has no associated rules. */
-static inline void audit_detach_watch(struct audit_krule *krule)
+/* Remove given krule from watch */
+static inline void audit_remove_watch_krule(struct audit_krule *krule)
 {
 	struct audit_watch *watch = krule->watch;
+	struct audit_parent *parent = krule->watch->parent;
+
+	audit_get_parent(parent);
+	audit_get_watch(watch);
+
+	down(&parent->watches_sem);
+	down(&watch->rules_sem);
 
-	list_del(&krule->rlist);
-	krule->watch = NULL;
+	if (!list_empty(&watch->rules))
+		list_del(&krule->rlist);
 
 	if (list_empty(&watch->rules)) {
-		list_del(&watch->mlist);
-		audit_free_watch(watch);
+		if (!list_empty(&parent->watches))
+			list_del(&watch->wlist);
+
+		if (list_empty(&parent->watches)) {
+			spin_lock(&master_parents_lock);
+			list_del(&parent->mlist);
+			spin_unlock(&master_parents_lock);
+
+			inotify_ignore(audit_idev, parent->wd);
+			audit_put_parent(parent);
+		}
+		audit_put_watch(watch);
 	}
+
+	up(&watch->rules_sem);
+	up(&parent->watches_sem);
+
+	audit_put_watch(watch);
+	audit_put_parent(parent);
 }
 
 /* Remove an existing rule from filterlist.  Protected by
@@ -433,19 +692,29 @@ static inline int audit_del_rule(struct 
 				 struct list_head *list)
 {
 	struct audit_entry  *e;
+	int ret = 0;
 
-	/* Do not use the _rcu iterator here, since this is the only
-	 * deletion routine. */
-	list_for_each_entry(e, list, list) {
-		if (!audit_compare_rule(&entry->rule, &e->rule)) {
-			list_del_rcu(&e->list);
-			if (e->rule.watch)
-				audit_detach_watch(&e->rule);
-			call_rcu(&e->rcu, audit_free_rule);
-			return 0;
-		}
+	/* The *_rcu iterator is needed to protect from filesystem
+	 * updates or removals. */
+	rcu_read_lock();
+	list_for_each_entry_rcu(e, list, list) {
+		if (audit_compare_rule(&entry->rule, &e->rule))
+			continue;
+
+		rcu_read_unlock();
+		if (e->rule.watch)
+			audit_remove_watch_krule(&e->rule);
+		list_del_rcu(&e->list);
+		call_rcu(&e->rcu, audit_free_rule_rcu);
+		goto out;
 	}
-	return -ENOENT;		/* No matching rule */
+	rcu_read_unlock();
+	ret = -ENOENT;		/* No matching rule */
+
+out:
+	if (entry->rule.watch)
+		audit_put_watch(entry->rule.watch);
+	return ret;
 }
 
 /* List rules using struct audit_rule.  Exists for backward
@@ -463,10 +732,11 @@ static int audit_list(void *_dest)
 
 	down(&audit_netlink_sem);
 
-	/* The *_rcu iterators not needed here because we are
-	   always called with audit_netlink_sem held. */
+	/* The *_rcu iterator is needed to protect from filesystem
+	 * updates or removals. */
 	for (i=0; i<AUDIT_NR_FILTERS; i++) {
-		list_for_each_entry(entry, &audit_filter_list[i], list) {
+		rcu_read_lock();
+		list_for_each_entry_rcu(entry, &audit_filter_list[i], list) {
 			struct audit_rule *rule;
 
 			rule = audit_krule_to_rule(&entry->rule);
@@ -476,6 +746,7 @@ static int audit_list(void *_dest)
 					 rule, sizeof(*rule));
 			kfree(rule);
 		}
+		rcu_read_unlock();
 	}
 	audit_send_reply(pid, seq, AUDIT_LIST, 1, 1, NULL, 0);
 	
@@ -497,10 +768,11 @@ static int audit_list_rules(void *_dest)
 
 	down(&audit_netlink_sem);
 
-	/* The *_rcu iterators not needed here because we are
-	   always called with audit_netlink_sem held. */
+	/* The *_rcu iterator is needed to protect from filesystem
+	 * updates or removals. */
 	for (i=0; i<AUDIT_NR_FILTERS; i++) {
-		list_for_each_entry(e, &audit_filter_list[i], list) {
+		rcu_read_lock();
+		list_for_each_entry_rcu(e, &audit_filter_list[i], list) {
 			struct audit_rule_data *data;
 
 			data = audit_krule_to_data(&e->rule);
@@ -510,6 +782,7 @@ static int audit_list_rules(void *_dest)
 					 data, sizeof(*data) + data->buflen);
 			kfree(data);
 		}
+		rcu_read_unlock();
 	}
 	audit_send_reply(pid, seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
 
@@ -574,11 +847,8 @@ int audit_receive_filter(int type, int p
 		if (!err)
 			audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
 				  "auid=%u added an audit rule\n", loginuid);
-		else {
-			if (entry->rule.watch)
-				audit_free_watch(entry->rule.watch);
-			kfree(entry);
-		}
+		else
+			audit_free_rule(entry);
 		break;
 	case AUDIT_DEL:
 	case AUDIT_DEL_RULE:
@@ -594,9 +864,7 @@ int audit_receive_filter(int type, int p
 		if (!err)
 			audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
 				  "auid=%u removed an audit rule\n", loginuid);
-		if (entry->rule.watch)
-			audit_free_watch(entry->rule.watch);
-		kfree(entry);
+		audit_free_rule(entry);
 		break;
 	default:
 		return -EINVAL;


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