[PATCH git] fix race conditions in filesystem audit patch

[Amy Griffis]

This patch fixes the following issues in the latest filesystem audit
patch:

    - fix error handling on call to audit_init_watch()
    - remove unnecessary check for watch in audit_add_rule()
    - in audit_del_rule(), don't check e->rule.watch after calling
      audit_free_rule_rcu
    - always update watch filter fields in audit_add_watch(), in case
      we don't find an existing watch struct
    - don't drop audit_filter_mutex between adding to existing
      watches/parents and adding the rule to the filterlist

If these fixes look correct, please fold them in with lspp.b7
f11fcf7556067b41d592d970f5f5c6ffbdd1e8fd on next rebase.

Signed-off-by: Amy Griffis <amy.griffis at hp.com>

---

 auditfilter.c |  144 ++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 86 insertions(+), 58 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f1151a2..eb102ff 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -585,9 +585,9 @@ static inline struct audit_watch *audit_
 		return ERR_PTR(-ENOMEM);
 
 	new = audit_init_watch(path);
-	if (unlikely(!new)) {
+	if (unlikely(IS_ERR(new))) {
 		kfree(path);
-		return ERR_PTR(-ENOMEM);
+		goto out;
 	}
 
 	new->dev = old->dev;
@@ -595,6 +595,7 @@ static inline struct audit_watch *audit_
 	audit_get_parent(old->parent);
 	new->parent = old->parent;
 
+out:
 	return new;
 }
 
@@ -853,77 +854,103 @@ static inline void audit_put_nd(struct n
 	}
 }
 
-/* Find a matching watch entry, or add this one. */
-static inline int audit_add_watch(struct audit_krule *krule,
-				  struct nameidata *ndp, struct nameidata *ndw,
- 				  struct list_head *inotify_list)
+/* Add a parent inotify_watch for the given rule. */
+static int audit_add_parent(struct audit_krule *krule,
+			    struct list_head *inotify_list)
+{
+	struct audit_parent *parent;
+	struct audit_watch *watch = krule->watch;
+
+	parent = audit_init_parent();
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	audit_get_parent(parent);
+	watch->parent = parent;
+
+	/* krule, watch and parent have not been added to any global
+	 * lists, so we don't need to take audit_filter_mutex. */
+	list_add(&watch->wlist, &parent->watches);
+	list_add(&krule->rlist, &watch->rules);
+
+	/* add parent to inotify registration list */
+	list_add(&parent->ilist, inotify_list);
+
+	return 0;
+}
+
+/* Associate the given rule with an existing parent inotify_watch.
+ * Caller must hold audit_filter_mutex. */
+static int audit_add_to_parent(struct audit_krule *krule,
+			       struct inotify_watch *iwatch)
 {
-	int ret;
-	struct inotify_watch *iwatch;
 	struct audit_parent *parent;
 	struct audit_watch *w, *watch = krule->watch;
 	int watch_found = 0;
 
-	ret = inotify_find_watch(audit_ih, ndp->dentry->d_inode, &iwatch);
-	if (ret < 0) {
-		parent = audit_init_parent();
-		if (IS_ERR(parent))
-			return PTR_ERR(parent);
+	parent = container_of(iwatch, struct audit_parent, wdata);
 
-		audit_get_parent(parent);
-		watch->parent = parent;
+	/* parent was moved before we took audit_filter_mutex */
+	if (parent->flags & AUDIT_PARENT_INVALID)
+		return -ENOENT;
 
-		/* krule, watch and parent have not been added to any global
-		 * lists, so we don't need to take audit_filter_mutex.
-		 */
-		list_add(&watch->wlist, &parent->watches);
-		list_add(&krule->rlist, &watch->rules);
+	list_for_each_entry(w, &parent->watches, wlist) {
+		if (strcmp(watch->path, w->path))
+			continue;
 
-		/* update watch filter fields */
-		if (ndw) {
-			watch->dev = ndw->dentry->d_inode->i_sb->s_dev;
-			watch->ino = ndw->dentry->d_inode->i_ino;
-		}
-		
-		/* add parent to inotify registration list */
-		list_add(&parent->ilist, inotify_list);
-	} else {
-		parent = container_of(iwatch, struct audit_parent, wdata);
+		watch_found = 1;
 
-		mutex_lock(&audit_filter_mutex);
-		if (parent->flags & AUDIT_PARENT_INVALID) {
-			/* parent was moved before we took the lock */
-			mutex_unlock(&audit_filter_mutex);
-			return -ENOENT;
-		}
+		/* put krule's and initial refs to temporary watch */
+		audit_put_watch(watch);
+		audit_put_watch(watch);
 
-		list_for_each_entry(w, &parent->watches, wlist) {
-			if (strcmp(watch->path, w->path))
-				continue;
+		audit_get_watch(w);
+		krule->watch = watch = w;
+		break;
+	}
 
-			watch_found = 1;
+	if (!watch_found) {
+		audit_get_parent(parent);
+		watch->parent = parent;
 
-			/* put krule's and initial refs to temporary watch */
-			audit_put_watch(watch);
-			audit_put_watch(watch);
+		list_add(&watch->wlist, &parent->watches);
+	}
 
-			audit_get_watch(w);
-			krule->watch = watch = w;
-			break;
-		}
+	list_add(&krule->rlist, &watch->rules);
 
-		if (!watch_found) {
-			audit_get_parent(parent);
-			watch->parent = parent;
+	return 0;
+}
 
-			list_add(&watch->wlist, &parent->watches);
-		}
+/* Find a matching watch entry, or add this one.
+ * Caller must hold audit_filter_mutex. */
+static int audit_add_watch(struct audit_krule *krule, struct nameidata *ndp,
+			   struct nameidata *ndw,
+			   struct list_head *inotify_list)
+{
+	struct audit_watch *watch = krule->watch;
+	struct inotify_watch *iwatch;
+	int ret;
 
-		list_add(&krule->rlist, &watch->rules);
-		mutex_unlock(&audit_filter_mutex);
+	/* update watch filter fields */
+	if (ndw) {
+		watch->dev = ndw->dentry->d_inode->i_sb->s_dev;
+		watch->ino = ndw->dentry->d_inode->i_ino;
 	}
 
-	return 0;
+	/* The audit_filter_mutex must not be held during inotify calls because
+	 * we hold it during inotify event callback processing.
+	 * We can trust iwatch to stick around because we hold nameidata (ndp). */
+	mutex_unlock(&audit_filter_mutex);
+
+	if (inotify_find_watch(audit_ih, ndp->dentry->d_inode, &iwatch) < 0) {
+		ret = audit_add_parent(krule, inotify_list);
+		mutex_lock(&audit_filter_mutex);
+	} else {
+		mutex_lock(&audit_filter_mutex);
+		ret = audit_add_to_parent(krule, iwatch);
+	}
+
+	return ret;
 }
 
 /* Add rule to given filterlist if not a duplicate. */
@@ -954,7 +981,9 @@ static inline int audit_add_rule(struct 
 			goto error;
 	}
 
+	mutex_lock(&audit_filter_mutex);
 	if (watch) {
+		/* audit_filter_mutex is dropped and re-taken during this call */
 		err = audit_add_watch(&entry->rule, ndp, ndw, &inotify_list);
 		if (err) {
 			audit_put_nd(ndp, ndw);
@@ -962,7 +991,6 @@ static inline int audit_add_rule(struct 
 		}
 	}
 
-	mutex_lock(&audit_filter_mutex);
 	if (entry->rule.flags & AUDIT_FILTER_PREPEND) {
 		list_add_rcu(&entry->list, list);
 	} else {
@@ -970,7 +998,7 @@ static inline int audit_add_rule(struct 
 	}
 	mutex_unlock(&audit_filter_mutex);
 
-	if (watch && !list_empty(&inotify_list)) {
+	if (!list_empty(&inotify_list)) {
 		err = audit_inotify_register(ndp, &inotify_list);
 		if (err)
 			goto error;
@@ -1031,7 +1059,7 @@ static inline int audit_del_rule(struct 
 		call_rcu(&e->rcu, audit_free_rule_rcu);
 		mutex_unlock(&audit_filter_mutex);
 
-		if (e->rule.watch && !list_empty(&inotify_list))
+		if (!list_empty(&inotify_list))
 			audit_inotify_unregister(&inotify_list);
 
 		return 0;