[PATCH 00/14] audit by executable name

[PATCH 00/14] audit by executable name

[Richard Guy Briggs]

This is a continuation of Peter Moody, my and Eric Paris' work to implement
audit by executable name.

Some of these are obvious.  Some demonstrate my lack of understanding of the
problem and of the services of fs/notify because they put needless restrictions
due to the orthogonal nature of the features involved or attempt to solve
problems that don't exist.  Posting this now to clarify some of that and move
on...

Eric Paris (3):
  audit: implement audit by executable
  audit: clean simple fsnotify implementation
  audit: convert audit_exe to audit_fsnotify

Richard Guy Briggs (11):
  fixup! audit: convert audit_exe to audit_fsnotify
  fixup! audit: clean simple fsnotify implementation
  audit: avoid double copying the audit_exe path string
  fixup! audit: convert audit_exe to audit_fsnotify
  fixup! audit: clean simple fsnotify implementation
  audit: put rule existence check in canonical order
  fixup! audit: implement audit by executable
  fixup! audit: implement audit by executable
  fixup! audit: clean simple fsnotify implementation
  audit: continue fleshing out audit by exe
  audit: enable audit_get/put_mark()

 include/linux/audit.h      |    2 +
 include/uapi/linux/audit.h |    2 +
 kernel/Makefile            |    2 +-
 kernel/audit.h             |   42 +++++++
 kernel/audit_exe.c         |   50 +++++++++
 kernel/audit_fsnotify.c    |  257 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/audit_tree.c        |    2 +-
 kernel/audit_watch.c       |    2 +-
 kernel/auditfilter.c       |   74 ++++++++++++-
 kernel/auditsc.c           |   16 +++
 10 files changed, 442 insertions(+), 7 deletions(-)
 create mode 100644 kernel/audit_exe.c
 create mode 100644 kernel/audit_fsnotify.c

[PATCH 01/14] audit: implement audit by executable

[Richard Guy Briggs]

From: Eric Paris <eparis@redhat.com>

This patch implements the ability to filter on the executable.  It is
clearly incomplete!  This patch adds the inode/dev of the executable at
the moment the rule is loaded.  It does not update if the executable is
updated/moved/whatever.  That should be added.  But at this moment, this
patch works.

Based-on-user-interface-by: Richard Guy Briggs <rgb@redhat.com>
Cc: rgb@redhat.com
Based-on-idea-by: Peter Moody <pmoody@google.com>
Cc: pmoody@google.com
Signed-off-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h      |    1 +
 include/uapi/linux/audit.h |    2 +
 kernel/Makefile            |    2 +-
 kernel/audit.h             |   27 ++++++++++
 kernel/audit_exe.c         |  113 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/auditfilter.c       |   43 +++++++++++++++++
 kernel/auditsc.c           |   16 ++++++
 7 files changed, 203 insertions(+), 1 deletions(-)
 create mode 100644 kernel/audit_exe.c

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 22cfddb..227171c 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -59,6 +59,7 @@ struct audit_krule {
 	struct audit_field	*inode_f; /* quick access to an inode field */
 	struct audit_watch	*watch;	/* associated watch */
 	struct audit_tree	*tree;	/* associated watched tree */
+	struct audit_exe	*exe;
 	struct list_head	rlist;	/* entry in audit_{watch,tree}.rules list */
 	struct list_head	list;	/* for AUDIT_LIST* purposes only */
 	u64			prio;
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 573dc36..f4a72b9 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -266,6 +266,8 @@
 #define AUDIT_OBJ_UID	109
 #define AUDIT_OBJ_GID	110
 #define AUDIT_FIELD_COMPARE	111
+#define AUDIT_EXE	112
+#define AUDIT_EXE_CHILDREN	113
 
 #define AUDIT_ARG0      200
 #define AUDIT_ARG1      (AUDIT_ARG0+1)
diff --git a/kernel/Makefile b/kernel/Makefile
index bc010ee..a1d5715 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_SMP) += stop_machine.o
 obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
 obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
 obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
-obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o
+obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o
 obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
 obj-$(CONFIG_GCOV_KERNEL) += gcov/
 obj-$(CONFIG_KPROBES) += kprobes.o
diff --git a/kernel/audit.h b/kernel/audit.h
index 7bb6573..58ed955 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -56,6 +56,7 @@ enum audit_state {
 
 /* Rule lists */
 struct audit_watch;
+struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
 
@@ -280,6 +281,13 @@ extern int audit_add_watch(struct audit_krule *krule, struct list_head **list);
 extern void audit_remove_watch_rule(struct audit_krule *krule);
 extern char *audit_watch_path(struct audit_watch *watch);
 extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t dev);
+
+int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op);
+void audit_remove_exe_rule(struct audit_krule *krule);
+char *audit_exe_path(struct audit_exe *exe);
+int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
+int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe);
+
 #else
 #define audit_put_watch(w) {}
 #define audit_get_watch(w) {}
@@ -289,6 +297,25 @@ extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev
 #define audit_watch_path(w) ""
 #define audit_watch_compare(w, i, d) 0
 
+static inline int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op) {
+	return -EINVAL;
+}
+static inline void audit_remove_exe_rule(struct audit_krule *krule) {
+	BUG();
+	return 0;
+}
+static inline char *audit_exe_path(struct audit_exe *exe) {
+	BUG();
+	return "";
+}
+static inline int audit_dup_exe(struct audit_krule *new, struct audit_krule *old) {
+	BUG();
+	return -EINVAL
+}
+static inline int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe) {
+	BUG();
+	return 0;
+}
 #endif /* CONFIG_AUDIT_WATCH */
 
 #ifdef CONFIG_AUDIT_TREE
diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
new file mode 100644
index 0000000..09c436c
--- /dev/null
+++ b/kernel/audit_exe.c
@@ -0,0 +1,113 @@
+/* audit_exe.c -- filtering of audit events
+ *
+ * Copyright 2014 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/audit.h>
+#include <linux/mutex.h>
+#include <linux/fs.h>
+#include <linux/namei.h>
+#include <linux/slab.h>
+#include "audit.h"
+
+struct audit_exe {
+	char *pathname;
+	unsigned long ino;
+	dev_t dev;
+};
+
+/* Translate a watch string to kernel respresentation. */
+int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op)
+{
+	struct audit_exe *exe;
+	struct path path;
+	struct dentry *dentry;
+	unsigned long ino;
+	dev_t dev;
+
+	if (pathname[0] != '/' || pathname[len-1] == '/')
+		return -EINVAL;
+
+	dentry = kern_path_locked(pathname, &path);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+	mutex_unlock(&path.dentry->d_inode->i_mutex);
+
+	if (!dentry->d_inode)
+		return -ENOENT;
+	dev = dentry->d_inode->i_sb->s_dev;
+	ino = dentry->d_inode->i_ino;
+	dput(dentry);
+
+	exe = kmalloc(sizeof(*exe), GFP_KERNEL);
+	if (!exe)
+		return -ENOMEM;
+	exe->ino = ino;
+	exe->dev = dev;
+	exe->pathname = pathname;
+	krule->exe = exe;
+
+	return 0;
+}
+
+void audit_remove_exe_rule(struct audit_krule *krule)
+{
+	struct audit_exe *exe;
+
+	exe = krule->exe;
+	krule->exe = NULL;
+	kfree(exe->pathname);
+	kfree(exe);
+}
+
+char *audit_exe_path(struct audit_exe *exe)
+{
+	return exe->pathname;
+}
+
+int audit_dup_exe(struct audit_krule *new, struct audit_krule *old)
+{
+	struct audit_exe *exe;
+
+	exe = kmalloc(sizeof(*exe), GFP_KERNEL);
+	if (!exe)
+		return -ENOMEM;
+
+	exe->pathname = kstrdup(old->exe->pathname, GFP_KERNEL);
+	if (!exe->pathname) {
+		kfree(exe);
+		return -ENOMEM;
+	}
+
+	exe->ino = old->exe->ino;
+	exe->dev = old->exe->dev;
+	new->exe = exe;
+
+	return 0;
+}
+
+int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
+{
+	if (tsk->mm->exe_file->f_inode->i_ino != exe->ino)
+		return 0;
+	if (tsk->mm->exe_file->f_inode->i_sb->s_dev != exe->dev)
+		return 0;
+	return 1;
+}
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 8e9bc9c..9caeaf5 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -390,6 +390,13 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
 		if (f->val > AUDIT_MAX_FIELD_COMPARE)
 			return -EINVAL;
 		break;
+	case AUDIT_EXE:
+	case AUDIT_EXE_CHILDREN:
+		if (f->op != Audit_equal)
+			return -EINVAL;
+		if (entry->rule.listnr != AUDIT_FILTER_EXIT)
+			return -EINVAL;
+		break;
 	};
 	return 0;
 }
@@ -541,6 +548,23 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			entry->rule.buflen += f->val;
 			entry->rule.filterkey = str;
 			break;
+		case AUDIT_EXE:
+		case AUDIT_EXE_CHILDREN:
+			if (entry->rule.exe || f->val > PATH_MAX)
+				goto exit_free;
+			str = audit_unpack_string(&bufp, &remain, f->val);
+			if (IS_ERR(str)) {
+				err = PTR_ERR(str);
+				goto exit_free;
+			}
+			entry->rule.buflen += f->val;
+
+			err = audit_make_exe_rule(&entry->rule, str, f->val, f->op);
+			if (err) {
+				kfree(str);
+				goto exit_free;
+			}
+			break;
 		}
 	}
 
@@ -619,6 +643,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
 			data->buflen += data->values[i] =
 				audit_pack_string(&bufp, krule->filterkey);
 			break;
+		case AUDIT_EXE:
+		case AUDIT_EXE_CHILDREN:
+			data->buflen += data->values[i] =
+				audit_pack_string(&bufp, audit_exe_path(krule->exe));
+			break;
 		default:
 			data->values[i] = f->val;
 		}
@@ -674,6 +703,13 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
 			if (strcmp(a->filterkey, b->filterkey))
 				return 1;
 			break;
+		case AUDIT_EXE:
+		case AUDIT_EXE_CHILDREN:
+			/* both paths exist based on above type compare */
+			if (strcmp(audit_exe_path(a->exe),
+				   audit_exe_path(b->exe)))
+				return 1;
+			break;
 		case AUDIT_UID:
 		case AUDIT_EUID:
 		case AUDIT_SUID:
@@ -795,6 +831,11 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
 				err = -ENOMEM;
 			else
 				new->filterkey = fk;
+			break;
+		case AUDIT_EXE:
+		case AUDIT_EXE_CHILDREN:
+			err = audit_dup_exe(new, old);
+			break;
 		}
 		if (err) {
 			audit_free_rule(entry);
@@ -966,6 +1007,8 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	if (e->rule.tree)
 		audit_remove_tree_rule(&e->rule);
 
+	if (e->rule.exe)
+		audit_remove_exe_rule(&e->rule);
 	list_del_rcu(&e->list);
 	list_del(&e->rule.list);
 	call_rcu(&e->rcu, audit_free_rule_rcu);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f251a5e..2513726 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -48,6 +48,7 @@
 #include <asm/types.h>
 #include <linux/atomic.h>
 #include <linux/fs.h>
+#include <linux/dcache.h>
 #include <linux/namei.h>
 #include <linux/mm.h>
 #include <linux/export.h>
@@ -70,6 +71,7 @@
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
+#include <linux/sched.h>
 #include <linux/ctype.h>
 
 #include "audit.h"
@@ -471,6 +473,20 @@ static int audit_filter_rules(struct task_struct *tsk,
 				result = audit_comparator(ctx->ppid, f->op, f->val);
 			}
 			break;
+		case AUDIT_EXE:
+			result = audit_exe_compare(tsk, rule->exe);
+			break;
+		case AUDIT_EXE_CHILDREN:
+		{
+			struct task_struct *ptsk;
+			for (ptsk = tsk; ptsk->parent->pid > 0; ptsk = find_task_by_vpid(ptsk->parent->pid)) {
+				if (audit_exe_compare(ptsk, rule->exe)) {
+					++result;
+					break;
+				}
+			}
+		}
+			break;
 		case AUDIT_UID:
 			result = audit_uid_comparator(cred->uid, f->op, f->uid);
 			break;
-- 
1.7.1

[PATCH 02/14] audit: clean simple fsnotify implementation

[Richard Guy Briggs]

From: Eric Paris <eparis@redhat.com>

This is to be used to audit by executable rules, but audit watches
should be able to share this code eventually.

At the moment the audit watch code is a lot more complex, that code only
creates one fsnotify watch per parent directory.  That 'audit_parent' in
turn has a list of 'audit_watches' which contain the name, ino, dev of
the specific object we care about.  This just creates one fsnotify watch
per object we care about.  So if you watch 100 inodes in /etc this code
will create 100 fsnotify watches on /etc.  The audit_watch code will
instead create 1 fsnotify watch on /etc (the audit_parent) and then 100
individual watches chained from that fsnotify mark.

We should be able to convert the audit_watch code to do one fsnotify
mark per watch and simplify things/remove a whole lot of code.  After
that conversion we should be able to convert the audit_fsnotify code to
support that hierarchy if the optomization is necessary.

Signed-off-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/Makefile         |    2 +-
 kernel/audit.h          |   29 ++++++
 kernel/audit_fsnotify.c |  251 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/auditfilter.c    |    2 +-
 4 files changed, 282 insertions(+), 2 deletions(-)
 create mode 100644 kernel/audit_fsnotify.c

diff --git a/kernel/Makefile b/kernel/Makefile
index a1d5715..32617ef 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_SMP) += stop_machine.o
 obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
 obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
 obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
-obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o
+obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o audit_fsnotify.o
 obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
 obj-$(CONFIG_GCOV_KERNEL) += gcov/
 obj-$(CONFIG_KPROBES) += kprobes.o
diff --git a/kernel/audit.h b/kernel/audit.h
index 58ed955..8d863d4 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -56,6 +56,7 @@ enum audit_state {
 
 /* Rule lists */
 struct audit_watch;
+struct audit_fsnotify_mark;
 struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
@@ -267,6 +268,7 @@ struct audit_net {
 extern int selinux_audit_rule_update(void);
 
 extern struct mutex audit_filter_mutex;
+extern int audit_del_rule(struct audit_entry *);
 extern void audit_free_rule_rcu(struct rcu_head *);
 extern struct list_head audit_filter_list[];
 
@@ -282,6 +284,11 @@ extern void audit_remove_watch_rule(struct audit_krule *krule);
 extern char *audit_watch_path(struct audit_watch *watch);
 extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t dev);
 
+struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct audit_krule *krule);
+char *audit_mark_path(struct audit_fsnotify_mark *mark);
+void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
+int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
+
 int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op);
 void audit_remove_exe_rule(struct audit_krule *krule);
 char *audit_exe_path(struct audit_exe *exe);
@@ -297,6 +304,28 @@ int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe);
 #define audit_watch_path(w) ""
 #define audit_watch_compare(w, i, d) 0
 
+static inline struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct audit_krule *krule)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline char *audit_mark_path(struct audit_fsnotify_mark *mark)
+{
+	BUG();
+	return "";
+}
+
+static inline void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
+{
+	BUG();
+}
+
+static inline int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev)
+{
+	BUG();
+	return 0;
+}
+
 static inline int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op) {
 	return -EINVAL;
 }
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
new file mode 100644
index 0000000..d0aa8f5
--- /dev/null
+++ b/kernel/audit_fsnotify.c
@@ -0,0 +1,251 @@
+/* audit_watch.c -- watching inodes
+ *
+ * Copyright 2003-2009 Red Hat, Inc.
+ * Copyright 2005 Hewlett-Packard Development Company, L.P.
+ * Copyright 2005 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/audit.h>
+#include <linux/kthread.h>
+#include <linux/mutex.h>
+#include <linux/fs.h>
+#include <linux/fsnotify_backend.h>
+#include <linux/namei.h>
+#include <linux/netlink.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/security.h>
+#include "audit.h"
+
+/*
+ * this mark lives on the parent directory of the inode in question.
+ * but dev, ino, and path are about the child
+ */
+struct audit_fsnotify_mark {
+	dev_t dev;		/* associated superblock device */
+	unsigned long ino;	/* associated inode number */
+	char *path;		/* insertion path */
+	struct fsnotify_mark mark; /* fsnotify mark on the inode */
+	struct audit_krule *rule;
+};
+
+/* fsnotify handle. */
+static struct fsnotify_group *audit_fsnotify_group;
+
+/* fsnotify events we care about. */
+#define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
+			 FS_MOVE_SELF | FS_EVENT_ON_CHILD)
+
+static void audit_free_mark(struct audit_fsnotify_mark *audit_mark)
+{
+	kfree(audit_mark->path);
+	kfree(audit_mark);
+}
+
+static void audit_free_fsnotify_mark(struct fsnotify_mark *mark)
+{
+	struct audit_fsnotify_mark *audit_mark;
+
+	audit_mark = container_of(mark, struct audit_fsnotify_mark, mark);
+	audit_free_mark(audit_mark);
+}
+
+#if 0 /* not sure if we need these... */
+static void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
+{
+	if (likely(audit_mark))
+		fsnotify_get_mark(&audit_mark->mark);
+}
+
+static void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
+{
+	if (likely(audit_mark))
+		fsnotify_put_mark(&audit_mark->mark);
+}
+#endif
+
+char *audit_mark_path(struct audit_fsnotify_mark *mark)
+{
+	return mark->path;
+}
+
+int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev)
+{
+	if (mark->ino == (unsigned long)-1)
+		return 0;
+	return ((mark->ino == ino) &&
+		(mark->dev == dev));
+}
+
+struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct audit_krule *krule)
+{
+	struct audit_fsnotify_mark *audit_mark;
+	struct path path;
+	struct dentry *dentry;
+	struct inode *inode;
+	unsigned long ino;
+	char *local_pathname;
+	dev_t dev;
+	int ret;
+
+	if (pathname[0] != '/' || pathname[len-1] == '/')
+		return ERR_PTR(-EINVAL);
+
+	dentry = kern_path_locked(pathname, &path);
+	if (IS_ERR(dentry))
+		return (void *)dentry; // returning an error
+	inode = path.dentry->d_inode;
+	mutex_unlock(&inode->i_mutex);
+
+	if (!dentry->d_inode) {
+		ino = (unsigned long)-1;
+		dev = (unsigned)-1;
+	} else {
+		dev = dentry->d_inode->i_sb->s_dev;
+		ino = dentry->d_inode->i_ino;
+	}
+
+	audit_mark = ERR_PTR(-ENOMEM);
+	local_pathname = kstrdup(pathname, GFP_KERNEL);
+	if (!local_pathname)
+		goto out;
+
+	audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
+	if (unlikely(!audit_mark)) {
+		kfree(local_pathname);
+		goto out;
+	}
+
+	fsnotify_init_mark(&audit_mark->mark, audit_free_fsnotify_mark);
+	audit_mark->mark.mask = AUDIT_FS_EVENTS;
+	audit_mark->path = local_pathname;
+	audit_mark->ino = ino;
+	audit_mark->dev = dev;
+	audit_mark->rule = krule;
+
+	ret = fsnotify_add_mark(&audit_mark->mark, audit_fsnotify_group, inode, NULL, true);
+	if (ret < 0) {
+		audit_free_mark(audit_mark);
+		audit_mark = ERR_PTR(ret);
+		goto out;
+	}
+out:
+	dput(dentry);
+	path_put(&path);
+	return audit_mark;
+}
+
+static void audit_watch_log_rule_change(struct audit_fsnotify_mark *audit_mark, char *op)
+{
+	struct audit_buffer *ab;
+	struct audit_krule *rule = audit_mark->rule;
+	if (!audit_enabled)
+		return;
+	ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+	if (unlikely(!ab))
+		return;
+	audit_log_format(ab, "auid=%u ses=%u op=",
+			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
+			 audit_get_sessionid(current));
+	audit_log_string(ab, op);
+	audit_log_format(ab, " path=");
+	audit_log_untrustedstring(ab, audit_mark->path);
+	audit_log_key(ab, rule->filterkey);
+	audit_log_format(ab, " list=%d res=1", rule->listnr);
+	audit_log_end(ab);
+}
+
+static int audit_update_mark(struct audit_fsnotify_mark *audit_mark,
+			     struct inode *inode)
+{
+	if (inode) {
+		audit_mark->dev = inode->i_sb->s_dev;
+		audit_mark->ino = inode->i_ino;
+	} else {
+		audit_mark->dev = (unsigned)-1;
+		audit_mark->ino = (unsigned long)-1;
+	}
+	return 0;
+}
+
+void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
+{
+	fsnotify_destroy_mark(&audit_mark->mark, audit_fsnotify_group);
+	fsnotify_put_mark(&audit_mark->mark);
+}
+
+static void audit_remove_rule(struct audit_fsnotify_mark *audit_mark)
+{
+	struct audit_krule *rule = audit_mark->rule;
+	struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
+
+	audit_watch_log_rule_change(audit_mark, "remove rule");
+	audit_del_rule(entry);
+}
+
+/* Update watch data in audit rules based on fsnotify events. */
+static int audit_watch_handle_event(struct fsnotify_group *group,
+				    struct inode *to_tell,
+				    struct fsnotify_mark *inode_mark,
+				    struct fsnotify_mark *vfsmount_mark,
+				    u32 mask, void *data, int data_type,
+				    const unsigned char *dname, u32 cookie)
+{
+	struct audit_fsnotify_mark *audit_mark;
+	struct inode *inode = NULL;
+
+	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
+
+	BUG_ON(group != audit_fsnotify_group);
+
+	switch (data_type) {
+	case (FSNOTIFY_EVENT_PATH):
+		inode = ((struct path *)data)->dentry->d_inode;
+		break;
+	case (FSNOTIFY_EVENT_INODE):
+		inode = (struct inode *)data;
+		break;
+	default:
+		BUG();
+		return 0;
+	};
+
+	if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
+		if (audit_compare_dname_path(dname, audit_mark->path, AUDIT_NAME_FULL))
+			return 0;
+		audit_update_mark(audit_mark, inode);
+	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
+		audit_remove_rule(audit_mark);
+
+	return 0;
+}
+
+static const struct fsnotify_ops audit_watch_fsnotify_ops = {
+	.handle_event =	audit_watch_handle_event,
+};
+
+static int __init audit_fsnotify_init(void)
+{
+	audit_fsnotify_group = fsnotify_alloc_group(&audit_watch_fsnotify_ops);
+	if (IS_ERR(audit_fsnotify_group)) {
+		audit_fsnotify_group = NULL;
+		audit_panic("cannot create audit fsnotify group");
+	}
+	return 0;
+}
+device_initcall(audit_fsnotify_init);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 9caeaf5..5c1951a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -977,7 +977,7 @@ error:
 }
 
 /* Remove an existing rule from filterlist. */
-static inline int audit_del_rule(struct audit_entry *entry)
+int audit_del_rule(struct audit_entry *entry)
 {
 	struct audit_entry  *e;
 	struct audit_watch *watch = entry->rule.watch;
-- 
1.7.1

[PATCH 03/14] audit: convert audit_exe to audit_fsnotify

[Richard Guy Briggs]

From: Eric Paris <eparis@redhat.com>

Instead of just hard coding the ino and dev of the executable we care
about at the moment the rule is inserted into the kernel, use the new
audit_fsnotify infrastructure.  This means that if the inode in question
is unlinked and creat'd (aka updated) the rule will just continue to
work.

Signed-off-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |    2 +-
 kernel/audit.h        |   31 ++++-------------
 kernel/audit_exe.c    |   87 +++++++------------------------------------------
 kernel/auditfilter.c  |   18 ++++++----
 4 files changed, 31 insertions(+), 107 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 227171c..f2a8044 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -59,7 +59,7 @@ struct audit_krule {
 	struct audit_field	*inode_f; /* quick access to an inode field */
 	struct audit_watch	*watch;	/* associated watch */
 	struct audit_tree	*tree;	/* associated watched tree */
-	struct audit_exe	*exe;
+	struct audit_fsnotify_mark	*exe;
 	struct list_head	rlist;	/* entry in audit_{watch,tree}.rules list */
 	struct list_head	list;	/* for AUDIT_LIST* purposes only */
 	u64			prio;
diff --git a/kernel/audit.h b/kernel/audit.h
index 8d863d4..61688ba 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -57,7 +57,6 @@ enum audit_state {
 /* Rule lists */
 struct audit_watch;
 struct audit_fsnotify_mark;
-struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
 
@@ -289,11 +288,8 @@ char *audit_mark_path(struct audit_fsnotify_mark *mark);
 void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
 int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
 
-int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op);
-void audit_remove_exe_rule(struct audit_krule *krule);
-char *audit_exe_path(struct audit_exe *exe);
 int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
-int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe);
+int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark);
 
 #else
 #define audit_put_watch(w) {}
@@ -320,31 +316,18 @@ static inline void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
 	BUG();
 }
 
-static inline int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev)
+static inline int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
 {
 	BUG();
-	return 0;
-}
-
-static inline int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op) {
 	return -EINVAL;
 }
-static inline void audit_remove_exe_rule(struct audit_krule *krule) {
-	BUG();
-	return 0;
-}
-static inline char *audit_exe_path(struct audit_exe *exe) {
-	BUG();
-	return "";
-}
-static inline int audit_dup_exe(struct audit_krule *new, struct audit_krule *old) {
-	BUG();
-	return -EINVAL
-}
-static inline int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe) {
+
+static inline int audit_dup_exe(struct audit_krule *new, struct audit_krule *old)
+{
 	BUG();
-	return 0;
+	return -EINVAL;
 }
+
 #endif /* CONFIG_AUDIT_WATCH */
 
 #ifdef CONFIG_AUDIT_TREE
diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
index 09c436c..d704a54 100644
--- a/kernel/audit_exe.c
+++ b/kernel/audit_exe.c
@@ -21,93 +21,30 @@
 
 #include <linux/kernel.h>
 #include <linux/audit.h>
-#include <linux/mutex.h>
 #include <linux/fs.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
 #include "audit.h"
 
-struct audit_exe {
-	char *pathname;
-	unsigned long ino;
-	dev_t dev;
-};
-
-/* Translate a watch string to kernel respresentation. */
-int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op)
-{
-	struct audit_exe *exe;
-	struct path path;
-	struct dentry *dentry;
-	unsigned long ino;
-	dev_t dev;
-
-	if (pathname[0] != '/' || pathname[len-1] == '/')
-		return -EINVAL;
-
-	dentry = kern_path_locked(pathname, &path);
-	if (IS_ERR(dentry))
-		return PTR_ERR(dentry);
-	mutex_unlock(&path.dentry->d_inode->i_mutex);
-
-	if (!dentry->d_inode)
-		return -ENOENT;
-	dev = dentry->d_inode->i_sb->s_dev;
-	ino = dentry->d_inode->i_ino;
-	dput(dentry);
-
-	exe = kmalloc(sizeof(*exe), GFP_KERNEL);
-	if (!exe)
-		return -ENOMEM;
-	exe->ino = ino;
-	exe->dev = dev;
-	exe->pathname = pathname;
-	krule->exe = exe;
-
-	return 0;
-}
-
-void audit_remove_exe_rule(struct audit_krule *krule)
-{
-	struct audit_exe *exe;
-
-	exe = krule->exe;
-	krule->exe = NULL;
-	kfree(exe->pathname);
-	kfree(exe);
-}
-
-char *audit_exe_path(struct audit_exe *exe)
-{
-	return exe->pathname;
-}
-
 int audit_dup_exe(struct audit_krule *new, struct audit_krule *old)
 {
-	struct audit_exe *exe;
-
-	exe = kmalloc(sizeof(*exe), GFP_KERNEL);
-	if (!exe)
-		return -ENOMEM;
+	struct audit_fsnotify_mark *audit_mark;
+	char *pathname;
 
-	exe->pathname = kstrdup(old->exe->pathname, GFP_KERNEL);
-	if (!exe->pathname) {
-		kfree(exe);
-		return -ENOMEM;
-	}
+	pathname = audit_mark_path(old->exe);
 
-	exe->ino = old->exe->ino;
-	exe->dev = old->exe->dev;
-	new->exe = exe;
+	audit_mark = audit_alloc_mark(pathname, strlen(pathname), new);
+	if (IS_ERR(audit_mark))
+		return PTR_ERR(audit_mark);
+	new->exe = audit_mark;
 
 	return 0;
 }
 
-int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
+int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
 {
-	if (tsk->mm->exe_file->f_inode->i_ino != exe->ino)
-		return 0;
-	if (tsk->mm->exe_file->f_inode->i_sb->s_dev != exe->dev)
-		return 0;
-	return 1;
+	unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
+	dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
+
+	return audit_mark_compare(mark, ino, dev);
 }
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 5c1951a..30091ce 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -411,6 +411,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 	size_t remain = datasz - sizeof(struct audit_rule_data);
 	int i;
 	char *str;
+	struct audit_fsnotify_mark *audit_mark;
 
 	entry = audit_to_entry_common(data);
 	if (IS_ERR(entry))
@@ -550,6 +551,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			break;
 		case AUDIT_EXE:
 		case AUDIT_EXE_CHILDREN:
+
 			if (entry->rule.exe || f->val > PATH_MAX)
 				goto exit_free;
 			str = audit_unpack_string(&bufp, &remain, f->val);
@@ -559,11 +561,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			}
 			entry->rule.buflen += f->val;
 
-			err = audit_make_exe_rule(&entry->rule, str, f->val, f->op);
-			if (err) {
-				kfree(str);
+			audit_mark = audit_alloc_mark(str, f->val, &entry->rule);
+			kfree(str);
+			if (IS_ERR(audit_mark)) {
+				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}
+			entry->rule.exe = audit_mark;
 			break;
 		}
 	}
@@ -646,7 +650,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
 		case AUDIT_EXE:
 		case AUDIT_EXE_CHILDREN:
 			data->buflen += data->values[i] =
-				audit_pack_string(&bufp, audit_exe_path(krule->exe));
+				audit_pack_string(&bufp, audit_mark_path(krule->exe));
 			break;
 		default:
 			data->values[i] = f->val;
@@ -706,8 +710,8 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
 		case AUDIT_EXE:
 		case AUDIT_EXE_CHILDREN:
 			/* both paths exist based on above type compare */
-			if (strcmp(audit_exe_path(a->exe),
-				   audit_exe_path(b->exe)))
+			if (strcmp(audit_mark_path(a->exe),
+				   audit_mark_path(b->exe)))
 				return 1;
 			break;
 		case AUDIT_UID:
@@ -1008,7 +1012,7 @@ int audit_del_rule(struct audit_entry *entry)
 		audit_remove_tree_rule(&e->rule);
 
 	if (e->rule.exe)
-		audit_remove_exe_rule(&e->rule);
+		audit_remove_mark(e->rule.exe);
 	list_del_rcu(&e->list);
 	list_del(&e->rule.list);
 	call_rcu(&e->rcu, audit_free_rule_rcu);
-- 
1.7.1

[PATCH 04/14] fixup! audit: convert audit_exe to audit_fsnotify

[Richard Guy Briggs]

Remove unnecessary space.
---
 kernel/auditfilter.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 30091ce..94b6af1 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -551,7 +551,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			break;
 		case AUDIT_EXE:
 		case AUDIT_EXE_CHILDREN:
-
 			if (entry->rule.exe || f->val > PATH_MAX)
 				goto exit_free;
 			str = audit_unpack_string(&bufp, &remain, f->val);
-- 
1.7.1

Re: [PATCH 04/14] fixup! audit: convert audit_exe to audit_fsnotify

[Eric Paris]

On Tue, 17 Jun 2014 23:09:39 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> Remove unnecessary space.
> ---
>  kernel/auditfilter.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 30091ce..94b6af1 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -551,7 +551,6 @@ static struct audit_entry
> *audit_data_to_entry(struct audit_rule_data *data, break;
>  		case AUDIT_EXE:
>  		case AUDIT_EXE_CHILDREN:
> -
>  			if (entry->rule.exe || f->val > PATH_MAX)
>  				goto exit_free;
>  			str = audit_unpack_string(&bufp, &remain,
> f->val);

For patches like these, would you rather I just fold it into my patch
to keep a clean history, or would you rather I apply this little
whitespace change? (obviously fixing things like the subject)

Re: [PATCH 04/14] fixup! audit: convert audit_exe to audit_fsnotify

[Richard Guy Briggs]

On 14/06/18, Eric Paris wrote:
> On Tue, 17 Jun 2014 23:09:39 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
> 
> > Remove unnecessary space.
> > ---
> >  kernel/auditfilter.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 30091ce..94b6af1 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -551,7 +551,6 @@ static struct audit_entry
> > *audit_data_to_entry(struct audit_rule_data *data, break;
> >  		case AUDIT_EXE:
> >  		case AUDIT_EXE_CHILDREN:
> > -
> >  			if (entry->rule.exe || f->val > PATH_MAX)
> >  				goto exit_free;
> >  			str = audit_unpack_string(&bufp, &remain,
> > f->val);
> 
> For patches like these, would you rather I just fold it into my patch
> to keep a clean history, or would you rather I apply this little
> whitespace change? (obviously fixing things like the subject)

Well, it was labelled to simply be --autosquash(ed) by git rebase -i, so
I'll just let git do that work later.

I was intending to shepherd this patchset through...

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[PATCH 05/14] fixup! audit: clean simple fsnotify implementation

[Richard Guy Briggs]

Rename several "watch" references to "mark".
---
 kernel/audit_fsnotify.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index d0aa8f5..707df2b 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -150,7 +150,7 @@ out:
 	return audit_mark;
 }
 
-static void audit_watch_log_rule_change(struct audit_fsnotify_mark *audit_mark, char *op)
+static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, char *op)
 {
 	struct audit_buffer *ab;
 	struct audit_krule *rule = audit_mark->rule;
@@ -194,12 +194,12 @@ static void audit_remove_rule(struct audit_fsnotify_mark *audit_mark)
 	struct audit_krule *rule = audit_mark->rule;
 	struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
 
-	audit_watch_log_rule_change(audit_mark, "remove rule");
+	audit_mark_log_rule_change(audit_mark, "remove rule");
 	audit_del_rule(entry);
 }
 
 /* Update watch data in audit rules based on fsnotify events. */
-static int audit_watch_handle_event(struct fsnotify_group *group,
+static int audit_mark_handle_event(struct fsnotify_group *group,
 				    struct inode *to_tell,
 				    struct fsnotify_mark *inode_mark,
 				    struct fsnotify_mark *vfsmount_mark,
@@ -235,13 +235,13 @@ static int audit_watch_handle_event(struct fsnotify_group *group,
 	return 0;
 }
 
-static const struct fsnotify_ops audit_watch_fsnotify_ops = {
-	.handle_event =	audit_watch_handle_event,
+static const struct fsnotify_ops audit_mark_fsnotify_ops = {
+	.handle_event =	audit_mark_handle_event,
 };
 
 static int __init audit_fsnotify_init(void)
 {
-	audit_fsnotify_group = fsnotify_alloc_group(&audit_watch_fsnotify_ops);
+	audit_fsnotify_group = fsnotify_alloc_group(&audit_mark_fsnotify_ops);
 	if (IS_ERR(audit_fsnotify_group)) {
 		audit_fsnotify_group = NULL;
 		audit_panic("cannot create audit fsnotify group");
-- 
1.7.1

[PATCH 06/14] audit: avoid double copying the audit_exe path string

[Richard Guy Briggs]

---
 kernel/audit_fsnotify.c |   12 ++----------
 kernel/auditfilter.c    |    2 +-
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 707df2b..07ebbbb 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -99,7 +99,6 @@ struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct aud
 	struct dentry *dentry;
 	struct inode *inode;
 	unsigned long ino;
-	char *local_pathname;
 	dev_t dev;
 	int ret;
 
@@ -120,20 +119,13 @@ struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct aud
 		ino = dentry->d_inode->i_ino;
 	}
 
-	audit_mark = ERR_PTR(-ENOMEM);
-	local_pathname = kstrdup(pathname, GFP_KERNEL);
-	if (!local_pathname)
-		goto out;
-
 	audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
-	if (unlikely(!audit_mark)) {
-		kfree(local_pathname);
+	if (unlikely(!audit_mark))
 		goto out;
-	}
 
 	fsnotify_init_mark(&audit_mark->mark, audit_free_fsnotify_mark);
 	audit_mark->mark.mask = AUDIT_FS_EVENTS;
-	audit_mark->path = local_pathname;
+	audit_mark->path = pathname;
 	audit_mark->ino = ino;
 	audit_mark->dev = dev;
 	audit_mark->rule = krule;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 94b6af1..5679b61 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -561,8 +561,8 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			entry->rule.buflen += f->val;
 
 			audit_mark = audit_alloc_mark(str, f->val, &entry->rule);
-			kfree(str);
 			if (IS_ERR(audit_mark)) {
+				kfree(str);
 				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}
-- 
1.7.1

[PATCH 07/14] fixup! audit: convert audit_exe to audit_fsnotify

[Richard Guy Briggs]

Put audit_alloc_mark() arguments in same order as watch, tree and inode.
---
 kernel/audit.h          |    2 +-
 kernel/audit_exe.c      |    2 +-
 kernel/audit_fsnotify.c |    2 +-
 kernel/auditfilter.c    |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index 61688ba..7bf3138 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -283,7 +283,7 @@ extern void audit_remove_watch_rule(struct audit_krule *krule);
 extern char *audit_watch_path(struct audit_watch *watch);
 extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t dev);
 
-struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct audit_krule *krule);
+struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len);
 char *audit_mark_path(struct audit_fsnotify_mark *mark);
 void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
 int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
index d704a54..42c6f55 100644
--- a/kernel/audit_exe.c
+++ b/kernel/audit_exe.c
@@ -33,7 +33,7 @@ int audit_dup_exe(struct audit_krule *new, struct audit_krule *old)
 
 	pathname = audit_mark_path(old->exe);
 
-	audit_mark = audit_alloc_mark(pathname, strlen(pathname), new);
+	audit_mark = audit_alloc_mark(new, pathname, strlen(pathname));
 	if (IS_ERR(audit_mark))
 		return PTR_ERR(audit_mark);
 	new->exe = audit_mark;
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 07ebbbb..0fda71f 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -92,7 +92,7 @@ int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_
 		(mark->dev == dev));
 }
 
-struct audit_fsnotify_mark *audit_alloc_mark(char *pathname, int len, struct audit_krule *krule)
+struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len)
 {
 	struct audit_fsnotify_mark *audit_mark;
 	struct path path;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 5679b61..c52cbc0 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -560,7 +560,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			}
 			entry->rule.buflen += f->val;
 
-			audit_mark = audit_alloc_mark(str, f->val, &entry->rule);
+			audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
 			if (IS_ERR(audit_mark)) {
 				kfree(str);
 				err = PTR_ERR(audit_mark);
-- 
1.7.1

[PATCH 08/14] fixup! audit: clean simple fsnotify implementation

[Richard Guy Briggs]

Remove redundant goto.
---
 kernel/audit_fsnotify.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 0fda71f..d169326 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -134,7 +134,6 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa
 	if (ret < 0) {
 		audit_free_mark(audit_mark);
 		audit_mark = ERR_PTR(ret);
-		goto out;
 	}
 out:
 	dput(dentry);
-- 
1.7.1

[PATCH 09/14] audit: put rule existence check in canonical order

[Richard Guy Briggs]

---
 kernel/auditfilter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index c52cbc0..cae8eae 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -148,7 +148,7 @@ static inline int audit_to_inode(struct audit_krule *krule,
 				 struct audit_field *f)
 {
 	if (krule->listnr != AUDIT_FILTER_EXIT ||
-	    krule->watch || krule->inode_f || krule->tree ||
+	    krule->inode_f || krule->watch || krule->tree ||
 	    (f->op != Audit_equal && f->op != Audit_not_equal))
 		return -EINVAL;
 
-- 
1.7.1

[PATCH 10/14] fixup! audit: implement audit by executable

[Richard Guy Briggs]

Check for existence of exe rule.
---
 kernel/audit_tree.c  |    2 +-
 kernel/audit_watch.c |    2 +-
 kernel/auditfilter.c |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 135944a..b4bf5d2 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -632,7 +632,7 @@ int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
 	if (pathname[0] != '/' ||
 	    rule->listnr != AUDIT_FILTER_EXIT ||
 	    op != Audit_equal ||
-	    rule->inode_f || rule->watch || rule->tree)
+	    rule->inode_f || rule->watch || rule->exe || rule->tree)
 		return -EINVAL;
 	rule->tree = alloc_tree(pathname);
 	if (!rule->tree)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 70b4554..1169de3 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -196,7 +196,7 @@ int audit_to_watch(struct audit_krule *krule, char *path, int len, u32 op)
 	if (path[0] != '/' || path[len-1] == '/' ||
 	    krule->listnr != AUDIT_FILTER_EXIT ||
 	    op != Audit_equal ||
-	    krule->inode_f || krule->watch || krule->tree)
+	    krule->inode_f || krule->watch || krule->exe || krule->tree)
 		return -EINVAL;
 
 	watch = audit_init_watch(path);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index cae8eae..eede673 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -148,7 +148,7 @@ static inline int audit_to_inode(struct audit_krule *krule,
 				 struct audit_field *f)
 {
 	if (krule->listnr != AUDIT_FILTER_EXIT ||
-	    krule->inode_f || krule->watch || krule->tree ||
+	    krule->inode_f || krule->watch || krule->exe || krule->tree ||
 	    (f->op != Audit_equal && f->op != Audit_not_equal))
 		return -EINVAL;
 
@@ -1423,7 +1423,7 @@ static int update_lsm_rule(struct audit_krule *r)
 		list_del_rcu(&entry->list);
 		list_del(&r->list);
 	} else {
-		if (r->watch || r->tree)
+		if (r->watch || r->exe || r->tree)
 			list_replace_init(&r->rlist, &nentry->rule.rlist);
 		list_replace_rcu(&entry->list, &nentry->list);
 		list_replace(&r->list, &nentry->rule.list);
-- 
1.7.1

[PATCH 11/14] fixup! audit: implement audit by executable

[Richard Guy Briggs]

Add space for consistency.
---
 kernel/auditfilter.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index eede673..f40c13b 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1012,6 +1012,7 @@ int audit_del_rule(struct audit_entry *entry)
 
 	if (e->rule.exe)
 		audit_remove_mark(e->rule.exe);
+
 	list_del_rcu(&e->list);
 	list_del(&e->rule.list);
 	call_rcu(&e->rcu, audit_free_rule_rcu);
-- 
1.7.1

[PATCH 12/14] fixup! audit: clean simple fsnotify implementation

[Richard Guy Briggs]

Rename audit_remove_rule() to audit_remove_mark_rule().
---
 kernel/audit_fsnotify.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index d169326..efefa16 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -180,7 +180,7 @@ void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
 	fsnotify_put_mark(&audit_mark->mark);
 }
 
-static void audit_remove_rule(struct audit_fsnotify_mark *audit_mark)
+static void audit_remove_mark_rule(struct audit_fsnotify_mark *audit_mark)
 {
 	struct audit_krule *rule = audit_mark->rule;
 	struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
@@ -221,7 +221,7 @@ static int audit_mark_handle_event(struct fsnotify_group *group,
 			return 0;
 		audit_update_mark(audit_mark, inode);
 	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
-		audit_remove_rule(audit_mark);
+		audit_remove_mark_rule(audit_mark);
 
 	return 0;
 }
-- 
1.7.1

[PATCH 13/14] audit: continue fleshing out audit by exe

[Richard Guy Briggs]

---
 include/linux/audit.h   |    1 +
 kernel/audit.h          |    1 +
 kernel/audit_fsnotify.c |   15 +++++++++++++++
 kernel/auditfilter.c    |   21 ++++++++++++++++++++-
 4 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index f2a8044..0bb9ea6 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -43,6 +43,7 @@ struct mq_attr;
 struct mqstat;
 struct audit_watch;
 struct audit_tree;
+struct audit_fsnotify_mark;
 struct sk_buff;
 
 struct audit_krule {
diff --git a/kernel/audit.h b/kernel/audit.h
index 7bf3138..2093c5e 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -285,6 +285,7 @@ extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev
 
 struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len);
 char *audit_mark_path(struct audit_fsnotify_mark *mark);
+int audit_add_mark_rule(struct audit_krule *krule, struct list_head **list);
 void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
 int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
 
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index efefa16..cc4175a 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -161,6 +161,21 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
 	audit_log_end(ab);
 }
 
+int audit_add_mark_rule(struct audit_krule *krule, struct list_head **list)
+{
+	struct audit_fsnotify_mark *audit_mark;
+	int h, ret = 0;
+
+	if (krule->exe)
+		audit_mark = krule->exe;
+	else
+		return -EINVAL;  //XXX
+
+	h = audit_hash_ino((u32)audit_mark->ino);
+	*list = &audit_inode_hash[h];
+	return ret;
+}
+
 static int audit_update_mark(struct audit_fsnotify_mark *audit_mark,
 			     struct inode *inode)
 {
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f40c13b..7b6e892 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -34,6 +34,7 @@
 #include <net/net_namespace.h>
 #include <net/sock.h>
 #include "audit.h"
+#include <linux/fsnotify_backend.h>
 
 /*
  * Locking model:
@@ -79,6 +80,8 @@ static inline void audit_free_rule(struct audit_entry *e)
 	/* some rules don't have associated watches */
 	if (erule->watch)
 		audit_put_watch(erule->watch);
+	if (erule->exe)
+		fsnotify_put_mark(erule->exe->mark);
 	if (erule->fields)
 		for (i = 0; i < erule->field_count; i++) {
 			struct audit_field *f = &erule->fields[i];
@@ -566,6 +569,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}
+			fsnotify_get_mark(audit_mark->mark);
 			entry->rule.exe = audit_mark;
 			break;
 		}
@@ -582,6 +586,8 @@ exit_free:
 		audit_put_watch(entry->rule.watch); /* matches initial get */
 	if (entry->rule.tree)
 		audit_put_tree(entry->rule.tree); /* that's the temporary one */
+	if (entry->rule.exe)
+		fsnotify_put_mark(entry->rule.exe->mark); /* matches initial get */
 	audit_free_rule(entry);
 	return ERR_PTR(err);
 }
@@ -866,7 +872,7 @@ static struct audit_entry *audit_find_rule(struct audit_entry *entry,
 	if (entry->rule.inode_f) {
 		h = audit_hash_ino(entry->rule.inode_f->val);
 		*p = list = &audit_inode_hash[h];
-	} else if (entry->rule.watch) {
+	} else if (entry->rule.watch || entry->rule.exe) {
 		/* we don't know the inode number, so must walk entire hash */
 		for (h = 0; h < AUDIT_INODE_BUCKETS; h++) {
 			list = &audit_inode_hash[h];
@@ -900,6 +906,7 @@ static inline int audit_add_rule(struct audit_entry *entry)
 	struct audit_entry *e;
 	struct audit_watch *watch = entry->rule.watch;
 	struct audit_tree *tree = entry->rule.tree;
+	struct audit_fsnotify_mark *exe = entry->rule.exe;
 	struct list_head *list;
 	int err;
 #ifdef CONFIG_AUDITSYSCALL
@@ -943,6 +950,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
 			goto error;
 		}
 	}
+	if (exe) {
+		err = audit_add_mark_rule(&entry->rule, &list);
+		if (err) {
+			mutex_unlock(&audit_filter_mutex);
+			goto error;
+		}
+	}
 
 	entry->rule.prio = ~0ULL;
 	if (entry->rule.listnr == AUDIT_FILTER_EXIT) {
@@ -976,6 +990,8 @@ static inline int audit_add_rule(struct audit_entry *entry)
 error:
 	if (watch)
 		audit_put_watch(watch); /* tmp watch, matches initial get */
+	if (exe)
+		fsnotify_put_mark(exe->mark); /* tmp mark, matches initial get */
 	return err;
 }
 
@@ -985,6 +1001,7 @@ int audit_del_rule(struct audit_entry *entry)
 	struct audit_entry  *e;
 	struct audit_watch *watch = entry->rule.watch;
 	struct audit_tree *tree = entry->rule.tree;
+	struct audit_fsnotify_mark *exe = entry->rule.exe;
 	struct list_head *list;
 	int ret = 0;
 #ifdef CONFIG_AUDITSYSCALL
@@ -1031,6 +1048,8 @@ out:
 		audit_put_watch(watch); /* match initial get */
 	if (tree)
 		audit_put_tree(tree);	/* that's the temporary one */
+	if (exe)
+		fsnotify_put_mark(exe->mark);	/* match initial get */
 
 	return ret;
 }
-- 
1.7.1

Re: [PATCH 13/14] audit: continue fleshing out audit by exe

[Eric Paris]

Whew, lot going on in here....

On Tue, 17 Jun 2014 23:09:48 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> ---
>  include/linux/audit.h   |    1 +
>  kernel/audit.h          |    1 +
>  kernel/audit_fsnotify.c |   15 +++++++++++++++
>  kernel/auditfilter.c    |   21 ++++++++++++++++++++-
>  4 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f2a8044..0bb9ea6 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -43,6 +43,7 @@ struct mq_attr;
>  struct mqstat;
>  struct audit_watch;
>  struct audit_tree;
> +struct audit_fsnotify_mark;
>  struct sk_buff;
>  
>  struct audit_krule {
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 7bf3138..2093c5e 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -285,6 +285,7 @@ extern int audit_watch_compare(struct audit_watch
> *watch, unsigned long ino, dev 
>  struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule
> *krule, char *pathname, int len); char *audit_mark_path(struct
> audit_fsnotify_mark *mark); +int audit_add_mark_rule(struct
> audit_krule *krule, struct list_head **list); void
> audit_remove_mark(struct audit_fsnotify_mark *audit_mark); int
> audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long
> ino, dev_t dev); diff --git a/kernel/audit_fsnotify.c
> b/kernel/audit_fsnotify.c index efefa16..cc4175a 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -161,6 +161,21 @@ static void audit_mark_log_rule_change(struct
> audit_fsnotify_mark *audit_mark, c audit_log_end(ab);
>  }
>  
> +int audit_add_mark_rule(struct audit_krule *krule, struct list_head
> **list) +{
> +	struct audit_fsnotify_mark *audit_mark;
> +	int h, ret = 0;
> +
> +	if (krule->exe)
> +		audit_mark = krule->exe;
> +	else
> +		return -EINVAL;  //XXX
> +
> +	h = audit_hash_ino((u32)audit_mark->ino);
> +	*list = &audit_inode_hash[h];
> +	return ret;
> +}

This would mean that audit_exe rules would trigger at the same times
audit_watch rules trigger.  Not sure if that is the semantics we are
after...


> +
>  static int audit_update_mark(struct audit_fsnotify_mark *audit_mark,
>  			     struct inode *inode)
>  {
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f40c13b..7b6e892 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -34,6 +34,7 @@
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
>  #include "audit.h"
> +#include <linux/fsnotify_backend.h>
>  
>  /*
>   * Locking model:
> @@ -79,6 +80,8 @@ static inline void audit_free_rule(struct
> audit_entry *e) /* some rules don't have associated watches */
>  	if (erule->watch)
>  		audit_put_watch(erule->watch);
> +	if (erule->exe)
> +		fsnotify_put_mark(erule->exe->mark);

Now this might be a good idea...  This is how marks would get cleaned
up in some error cases, whereas I believe it would get cleaned up from
rule removal in audit_remove_mark()

>  	if (erule->fields)
>  		for (i = 0; i < erule->field_count; i++) {
>  			struct audit_field *f = &erule->fields[i];
> @@ -566,6 +569,7 @@ static struct audit_entry
> *audit_data_to_entry(struct audit_rule_data *data, err =
> PTR_ERR(audit_mark); goto exit_free;
>  			}
> +			fsnotify_get_mark(audit_mark->mark);

So this just took a reference on the mark, so now the refcnt is 2.
Even though there is only 1 user...  (what else references audit_mark
other than entry->rule.exe?)

>  			entry->rule.exe = audit_mark;
>  			break;
>  		}
> @@ -582,6 +586,8 @@ exit_free:
>  		audit_put_watch(entry->rule.watch); /* matches
> initial get */ if (entry->rule.tree)
>  		audit_put_tree(entry->rule.tree); /* that's the
> temporary one */
> +	if (entry->rule.exe)
> +		fsnotify_put_mark(entry->rule.exe->mark); /* matches
> initial get */ audit_free_rule(entry);

ok, so maybe this doesn't panic, since you took and extra reference
above you can put a reference here and then again inside
audit_free_rule().  But the code is still wrong...

>  	return ERR_PTR(err);
>  }
> @@ -866,7 +872,7 @@ static struct audit_entry *audit_find_rule(struct
> audit_entry *entry, if (entry->rule.inode_f) {
>  		h = audit_hash_ino(entry->rule.inode_f->val);
>  		*p = list = &audit_inode_hash[h];
> -	} else if (entry->rule.watch) {
> +	} else if (entry->rule.watch || entry->rule.exe) {

nope

>  		/* we don't know the inode number, so must walk
> entire hash */ for (h = 0; h < AUDIT_INODE_BUCKETS; h++) {
>  			list = &audit_inode_hash[h];
> @@ -900,6 +906,7 @@ static inline int audit_add_rule(struct
> audit_entry *entry) struct audit_entry *e;
>  	struct audit_watch *watch = entry->rule.watch;
>  	struct audit_tree *tree = entry->rule.tree;
> +	struct audit_fsnotify_mark *exe = entry->rule.exe;
>  	struct list_head *list;
>  	int err;
>  #ifdef CONFIG_AUDITSYSCALL
> @@ -943,6 +950,13 @@ static inline int audit_add_rule(struct
> audit_entry *entry) goto error;
>  		}
>  	}
> +	if (exe) {
> +		err = audit_add_mark_rule(&entry->rule, &list);
> +		if (err) {
> +			mutex_unlock(&audit_filter_mutex);
> +			goto error;
> +		}
> +	}

naah, we don't want to find exe rules when we are looking for watch
rules.  This is the list of things we are watching for FS operations
like open, chmod, chown, etc.  Not things we are exec'ing...

>  
>  	entry->rule.prio = ~0ULL;
>  	if (entry->rule.listnr == AUDIT_FILTER_EXIT) {
> @@ -976,6 +990,8 @@ static inline int audit_add_rule(struct
> audit_entry *entry) error:
>  	if (watch)
>  		audit_put_watch(watch); /* tmp watch, matches
> initial get */
> +	if (exe)
> +		fsnotify_put_mark(exe->mark); /* tmp mark, matches
> initial get */ return err;

I'm not so sure the 'watch' code is right here, since any failure is
going to call audit_free_rule(), which will free the watch (and which
you 'fixed' to put the exe...

>  }
>  
> @@ -985,6 +1001,7 @@ int audit_del_rule(struct audit_entry *entry)
>  	struct audit_entry  *e;
>  	struct audit_watch *watch = entry->rule.watch;
>  	struct audit_tree *tree = entry->rule.tree;
> +	struct audit_fsnotify_mark *exe = entry->rule.exe;
>  	struct list_head *list;
>  	int ret = 0;
>  #ifdef CONFIG_AUDITSYSCALL
> @@ -1031,6 +1048,8 @@ out:
>  		audit_put_watch(watch); /* match initial get */
>  	if (tree)
>  		audit_put_tree(tree);	/* that's the temporary
> one */
> +	if (exe)
> +		fsnotify_put_mark(exe->mark);	/* match
> initial get */ 

So audit_del_rule() frees both the rule that was in the kernel being
used on a filter list and the rule that is passed to it used to find
the rule on the filter list?  This is a crap interface and needs
rewritten...

I'm pretty sure that's a bug in my code.

audit_del_rule() has 2 callers.
1) audit_rule_change - which takes userspace input, creates a new rule,
then passes that new rule to audit_del_rule() which find the rule on
the filter list and frees both of them (kinda sorta...)

2) audit_remove_rule() (which badly needs renamed) which actually finds
the rule ON the filter list and passes that to audit_del_rule().
Which finds itself and then frees itself twice (kinda sorta)

Yeah, refcounting on marks/trees seems really off to me...

Re: [PATCH 13/14] audit: continue fleshing out audit by exe

[Richard Guy Briggs]

On 14/06/18, Eric Paris wrote:
> Whew, lot going on in here....

Yeah, overloaded that commit...  I've split it...
(more below...)

> On Tue, 17 Jun 2014 23:09:48 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > --- a/kernel/audit_fsnotify.c
> > +++ b/kernel/audit_fsnotify.c
> > @@ -161,6 +161,21 @@ static void audit_mark_log_rule_change(struct
> > audit_fsnotify_mark *audit_mark, c audit_log_end(ab);
> >  }
> >  
> > +int audit_add_mark_rule(struct audit_krule *krule, struct list_head
> > **list) +{
> > +	struct audit_fsnotify_mark *audit_mark;
> > +	int h, ret = 0;
> > +
> > +	if (krule->exe)
> > +		audit_mark = krule->exe;
> > +	else
> > +		return -EINVAL;  //XXX
> > +
> > +	h = audit_hash_ino((u32)audit_mark->ino);
> > +	*list = &audit_inode_hash[h];
> > +	return ret;
> > +}
> 
> This would mean that audit_exe rules would trigger at the same times
> audit_watch rules trigger.  Not sure if that is the semantics we are
> after...

Yeah, I suspected I had misunderstood and was going down the wrong
path...

> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -79,6 +80,8 @@ static inline void audit_free_rule(struct audit_entry *e)
> >	/* some rules don't have associated watches */
> >  	if (erule->watch)
> >  		audit_put_watch(erule->watch);
> > +	if (erule->exe)
> > +		fsnotify_put_mark(erule->exe->mark);
> 
> Now this might be a good idea...  This is how marks would get cleaned
> up in some error cases, whereas I believe it would get cleaned up from
> rule removal in audit_remove_mark()

In fact, it looks to me like this should be
audit_remove_mark(erule->exe), and get rid of the audit_remove_mark()
from audit_del_rule().

This way, rules that get purged due to a watch disappearing will have
the mark properly cleaned up rather than orphaned.

> > @@ -566,6 +569,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> >				err = PTR_ERR(audit_mark);
> >				goto exit_free;
> >  			}
> > +			fsnotify_get_mark(audit_mark->mark);
> 
> So this just took a reference on the mark, so now the refcnt is 2.
> Even though there is only 1 user...  (what else references audit_mark
> other than entry->rule.exe?)

Actually, it was already 2 because of the call to fsnotify_add_mark()
which is bumped up for g_list and i_list (freed by the
fsnotify_mark_destroy thread), but yeah, this is unnecessary, I agree.

> > @@ -582,6 +586,8 @@ exit_free:
> >  		audit_put_watch(entry->rule.watch); /* matches initial get */
> >	if (entry->rule.tree)
> >  		audit_put_tree(entry->rule.tree); /* that's the temporary one */
> > +	if (entry->rule.exe)
> > +		fsnotify_put_mark(entry->rule.exe->mark); /* matches initial get */
> > 	audit_free_rule(entry);
> 
> ok, so maybe this doesn't panic, since you took and extra reference
> above you can put a reference here and then again inside
> audit_free_rule().  But the code is still wrong...

Agreed.

> > @@ -866,7 +872,7 @@ static struct audit_entry *audit_find_rule(struct audit_entry *entry,
> >	if (entry->rule.inode_f) {
> >  		h = audit_hash_ino(entry->rule.inode_f->val);
> >  		*p = list = &audit_inode_hash[h];
> > -	} else if (entry->rule.watch) {
> > +	} else if (entry->rule.watch || entry->rule.exe) {
> 
> nope

Agreed.

> > @@ -943,6 +950,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
> >			goto error;
> >  		}
> >  	}
> > +	if (exe) {
> > +		err = audit_add_mark_rule(&entry->rule, &list);
> > +		if (err) {
> > +			mutex_unlock(&audit_filter_mutex);
> > +			goto error;
> > +		}
> > +	}
> 
> naah, we don't want to find exe rules when we are looking for watch
> rules.  This is the list of things we are watching for FS operations
> like open, chmod, chown, etc.  Not things we are exec'ing...

Understood now.

> > @@ -976,6 +990,8 @@ static inline int audit_add_rule(struct audit_entry *entry)
> > error:
> >  	if (watch)
> >  		audit_put_watch(watch); /* tmp watch, matches initial get */
> > +	if (exe)
> > +		fsnotify_put_mark(exe->mark); /* tmp mark, matches initial get */
> 
> I'm not so sure the 'watch' code is right here, since any failure is
> going to call audit_free_rule(), which will free the watch (and which
> you 'fixed' to put the exe...

I am starting to suspect that myself.  Fixing that is a seperate patch,
but understanding that will help get this one right.

> > @@ -1031,6 +1048,8 @@ out:
> >  		audit_put_watch(watch); /* match initial get */
> >  	if (tree)
> >  		audit_put_tree(tree);	/* that's the temporary one */
> > +	if (exe)
> > +		fsnotify_put_mark(exe->mark);	/* match initial get */ 
> 
> So audit_del_rule() frees both the rule that was in the kernel being
> used on a filter list and the rule that is passed to it used to find
> the rule on the filter list?  This is a crap interface and needs
> rewritten...

Agreed.  So it should free the rule it is seeking in audit_del_rule()
and the rule passed to it should be freed after it returns.

> I'm pretty sure that's a bug in my code.
> 
> audit_del_rule() has 2 callers.
> 1) audit_rule_change - which takes userspace input, creates a new rule,
> then passes that new rule to audit_del_rule() which find the rule on
> the filter list and frees both of them (kinda sorta...)
> 
> 2) audit_remove_rule() (which badly needs renamed) which actually finds
> the rule ON the filter list and passes that to audit_del_rule().
> Which finds itself and then frees itself twice (kinda sorta)

Yes, this was renamed to audit_remove_mark_rule().  In there, the entry
should be sent to audit_free_rule() after returning from
audit_del_rule().

Along the way, I was thinking that fsnotify_destroy_mark_locked() should
call fsnotify_put_mark() as well, but I think I've convinced myself that
is a bad idea because either there is no other holder of that mark
reference, or the reference is still needed for other cleanup.

> Yeah, refcounting on marks/trees seems really off to me...

Suspecting that helps to understand this...

I'll look at the watch and tree count structures somewhat seperately.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[PATCH 14/14] audit: enable audit_get/put_mark()

[Richard Guy Briggs]

---
 kernel/audit.h          |    2 ++
 kernel/audit_fsnotify.c |    6 +++---
 kernel/auditfilter.c    |   10 +++++-----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index 2093c5e..3151ae5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -288,6 +288,8 @@ char *audit_mark_path(struct audit_fsnotify_mark *mark);
 int audit_add_mark_rule(struct audit_krule *krule, struct list_head **list);
 void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
 int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
+void audit_get_mark(struct audit_fsnotify_mark *audit_mark);
+void audit_put_mark(struct audit_fsnotify_mark *audit_mark);
 
 int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
 int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index cc4175a..f5789e1 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -65,14 +65,14 @@ static void audit_free_fsnotify_mark(struct fsnotify_mark *mark)
 	audit_free_mark(audit_mark);
 }
 
-#if 0 /* not sure if we need these... */
-static void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
+#if 1 /* not sure if we need these... */
+void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
 {
 	if (likely(audit_mark))
 		fsnotify_get_mark(&audit_mark->mark);
 }
 
-static void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
+void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
 {
 	if (likely(audit_mark))
 		fsnotify_put_mark(&audit_mark->mark);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 7b6e892..3d168ca 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -81,7 +81,7 @@ static inline void audit_free_rule(struct audit_entry *e)
 	if (erule->watch)
 		audit_put_watch(erule->watch);
 	if (erule->exe)
-		fsnotify_put_mark(erule->exe->mark);
+		audit_put_mark(erule->exe);
 	if (erule->fields)
 		for (i = 0; i < erule->field_count; i++) {
 			struct audit_field *f = &erule->fields[i];
@@ -569,7 +569,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}
-			fsnotify_get_mark(audit_mark->mark);
+			audit_get_mark(audit_mark);
 			entry->rule.exe = audit_mark;
 			break;
 		}
@@ -587,7 +587,7 @@ exit_free:
 	if (entry->rule.tree)
 		audit_put_tree(entry->rule.tree); /* that's the temporary one */
 	if (entry->rule.exe)
-		fsnotify_put_mark(entry->rule.exe->mark); /* matches initial get */
+		audit_put_mark(entry->rule.exe); /* matches initial get */
 	audit_free_rule(entry);
 	return ERR_PTR(err);
 }
@@ -991,7 +991,7 @@ error:
 	if (watch)
 		audit_put_watch(watch); /* tmp watch, matches initial get */
 	if (exe)
-		fsnotify_put_mark(exe->mark); /* tmp mark, matches initial get */
+		audit_put_mark(exe); /* tmp mark, matches initial get */
 	return err;
 }
 
@@ -1049,7 +1049,7 @@ out:
 	if (tree)
 		audit_put_tree(tree);	/* that's the temporary one */
 	if (exe)
-		fsnotify_put_mark(exe->mark);	/* match initial get */
+		audit_put_mark(exe);	/* match initial get */
 
 	return ret;
 }
-- 
1.7.1

Re: [PATCH 14/14] audit: enable audit_get/put_mark()

[Eric Paris]

Aside from the refcounting in the previous patch being bad, which you
likely didn't make any better here, I think I'm ok with
audit_put_mark/audit_get_mark being exposed and used instead of the
fsnotify_* calls being used directly...


On Tue, 17 Jun 2014 23:09:49 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> ---
>  kernel/audit.h          |    2 ++
>  kernel/audit_fsnotify.c |    6 +++---
>  kernel/auditfilter.c    |   10 +++++-----
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 2093c5e..3151ae5 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -288,6 +288,8 @@ char *audit_mark_path(struct audit_fsnotify_mark
> *mark); int audit_add_mark_rule(struct audit_krule *krule, struct
> list_head **list); void audit_remove_mark(struct audit_fsnotify_mark
> *audit_mark); int audit_mark_compare(struct audit_fsnotify_mark
> *mark, unsigned long ino, dev_t dev); +void audit_get_mark(struct
> audit_fsnotify_mark *audit_mark); +void audit_put_mark(struct
> audit_fsnotify_mark *audit_mark); 
>  int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
>  int audit_exe_compare(struct task_struct *tsk, struct
> audit_fsnotify_mark *mark); diff --git a/kernel/audit_fsnotify.c
> b/kernel/audit_fsnotify.c index cc4175a..f5789e1 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -65,14 +65,14 @@ static void audit_free_fsnotify_mark(struct
> fsnotify_mark *mark) audit_free_mark(audit_mark);
>  }
>  
> -#if 0 /* not sure if we need these... */
> -static void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
> +#if 1 /* not sure if we need these... */
> +void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
>  {
>  	if (likely(audit_mark))
>  		fsnotify_get_mark(&audit_mark->mark);
>  }
>  
> -static void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
> +void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
>  {
>  	if (likely(audit_mark))
>  		fsnotify_put_mark(&audit_mark->mark);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 7b6e892..3d168ca 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -81,7 +81,7 @@ static inline void audit_free_rule(struct
> audit_entry *e) if (erule->watch)
>  		audit_put_watch(erule->watch);
>  	if (erule->exe)
> -		fsnotify_put_mark(erule->exe->mark);
> +		audit_put_mark(erule->exe);
>  	if (erule->fields)
>  		for (i = 0; i < erule->field_count; i++) {
>  			struct audit_field *f = &erule->fields[i];
> @@ -569,7 +569,7 @@ static struct audit_entry
> *audit_data_to_entry(struct audit_rule_data *data, err =
> PTR_ERR(audit_mark); goto exit_free;
>  			}
> -			fsnotify_get_mark(audit_mark->mark);
> +			audit_get_mark(audit_mark);
>  			entry->rule.exe = audit_mark;
>  			break;
>  		}
> @@ -587,7 +587,7 @@ exit_free:
>  	if (entry->rule.tree)
>  		audit_put_tree(entry->rule.tree); /* that's the
> temporary one */ if (entry->rule.exe)
> -		fsnotify_put_mark(entry->rule.exe->mark); /* matches
> initial get */
> +		audit_put_mark(entry->rule.exe); /* matches initial
> get */ audit_free_rule(entry);
>  	return ERR_PTR(err);
>  }
> @@ -991,7 +991,7 @@ error:
>  	if (watch)
>  		audit_put_watch(watch); /* tmp watch, matches
> initial get */ if (exe)
> -		fsnotify_put_mark(exe->mark); /* tmp mark, matches
> initial get */
> +		audit_put_mark(exe); /* tmp mark, matches initial
> get */ return err;
>  }
>  
> @@ -1049,7 +1049,7 @@ out:
>  	if (tree)
>  		audit_put_tree(tree);	/* that's the temporary
> one */ if (exe)
> -		fsnotify_put_mark(exe->mark);	/* match
> initial get */
> +		audit_put_mark(exe);	/* match initial get */
>  
>  	return ret;
>  }

Re: [PATCH 14/14] audit: enable audit_get/put_mark()

[Richard Guy Briggs]

On 14/06/18, Eric Paris wrote:
> Aside from the refcounting in the previous patch being bad, which you
> likely didn't make any better here, I think I'm ok with
> audit_put_mark/audit_get_mark being exposed and used instead of the
> fsnotify_* calls being used directly...

I still need to convince myself they are needed, but I'll figure that
out after I read more carefully your comments to the previous patch.

> On Tue, 17 Jun 2014 23:09:49 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > ---
> >  kernel/audit.h          |    2 ++
> >  kernel/audit_fsnotify.c |    6 +++---
> >  kernel/auditfilter.c    |   10 +++++-----
> >  3 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 2093c5e..3151ae5 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -288,6 +288,8 @@ char *audit_mark_path(struct audit_fsnotify_mark
> > *mark); int audit_add_mark_rule(struct audit_krule *krule, struct
> > list_head **list); void audit_remove_mark(struct audit_fsnotify_mark
> > *audit_mark); int audit_mark_compare(struct audit_fsnotify_mark
> > *mark, unsigned long ino, dev_t dev); +void audit_get_mark(struct
> > audit_fsnotify_mark *audit_mark); +void audit_put_mark(struct
> > audit_fsnotify_mark *audit_mark); 
> >  int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
> >  int audit_exe_compare(struct task_struct *tsk, struct
> > audit_fsnotify_mark *mark); diff --git a/kernel/audit_fsnotify.c
> > b/kernel/audit_fsnotify.c index cc4175a..f5789e1 100644
> > --- a/kernel/audit_fsnotify.c
> > +++ b/kernel/audit_fsnotify.c
> > @@ -65,14 +65,14 @@ static void audit_free_fsnotify_mark(struct
> > fsnotify_mark *mark) audit_free_mark(audit_mark);
> >  }
> >  
> > -#if 0 /* not sure if we need these... */
> > -static void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
> > +#if 1 /* not sure if we need these... */
> > +void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
> >  {
> >  	if (likely(audit_mark))
> >  		fsnotify_get_mark(&audit_mark->mark);
> >  }
> >  
> > -static void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
> > +void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
> >  {
> >  	if (likely(audit_mark))
> >  		fsnotify_put_mark(&audit_mark->mark);
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 7b6e892..3d168ca 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -81,7 +81,7 @@ static inline void audit_free_rule(struct
> > audit_entry *e) if (erule->watch)
> >  		audit_put_watch(erule->watch);
> >  	if (erule->exe)
> > -		fsnotify_put_mark(erule->exe->mark);
> > +		audit_put_mark(erule->exe);
> >  	if (erule->fields)
> >  		for (i = 0; i < erule->field_count; i++) {
> >  			struct audit_field *f = &erule->fields[i];
> > @@ -569,7 +569,7 @@ static struct audit_entry
> > *audit_data_to_entry(struct audit_rule_data *data, err =
> > PTR_ERR(audit_mark); goto exit_free;
> >  			}
> > -			fsnotify_get_mark(audit_mark->mark);
> > +			audit_get_mark(audit_mark);
> >  			entry->rule.exe = audit_mark;
> >  			break;
> >  		}
> > @@ -587,7 +587,7 @@ exit_free:
> >  	if (entry->rule.tree)
> >  		audit_put_tree(entry->rule.tree); /* that's the
> > temporary one */ if (entry->rule.exe)
> > -		fsnotify_put_mark(entry->rule.exe->mark); /* matches
> > initial get */
> > +		audit_put_mark(entry->rule.exe); /* matches initial
> > get */ audit_free_rule(entry);
> >  	return ERR_PTR(err);
> >  }
> > @@ -991,7 +991,7 @@ error:
> >  	if (watch)
> >  		audit_put_watch(watch); /* tmp watch, matches
> > initial get */ if (exe)
> > -		fsnotify_put_mark(exe->mark); /* tmp mark, matches
> > initial get */
> > +		audit_put_mark(exe); /* tmp mark, matches initial
> > get */ return err;
> >  }
> >  
> > @@ -1049,7 +1049,7 @@ out:
> >  	if (tree)
> >  		audit_put_tree(tree);	/* that's the temporary
> > one */ if (exe)
> > -		fsnotify_put_mark(exe->mark);	/* match
> > initial get */
> > +		audit_put_mark(exe);	/* match initial get */
> >  
> >  	return ret;
> >  }
> 

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545