audit by executable name

From: Eric Paris <[email protected]>

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.

RGB: Explicitly declare prototypes as extern.
RGB: Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch, lsm_field.

Based-on-user-interface-by Richard Guy Briggs <[email protected]>

Based-on-idea-by Peter Moody <[email protected]>

Cc [email protected]

Signed-off-by Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

kernel/audit.h +15 -0

index d641f9b..3aca24f 100644
@@ -278,6 +286,30 @@ 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_dupe_exe(struct audit_krule *new, struct

kernel/audit_exe.c new file +33 -0

new file mode 100644
index 0000000..d4cc8b5
@@ -0,0 +1,109 @@
+/* audit_exe.c -- filtering of audit events
+ *
+ * Copyright 2014-2015 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.
+ */
+
+#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)

kernel/audit_exe.c new file +18 -0

+{
+	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;

kernel/audit_exe.c new file +24 -0

+char *audit_exe_path(struct audit_exe *exe)
+{
+	return exe->pathname;
+}
+
+int audit_dupe_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;

kernel/auditsc.c +7 -0

index 9fb9d1c..bf745c7 100644
@@ -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>

@@ -71,6 +72,7 @@
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
+#include <linux/sched.h>
 #include <linux/ctype.h>
 #include <linux/string.h>
 #include <uapi/linux/limits.h>

@@ -466,6 +468,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:
+		{

Quoted 7 lines · Richard Guy Briggs

From: Eric Paris <[email protected]>

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.

This needs work. Either this patch is incomplete as the description says, in which case I'm not going to merge it, or the description is out of date and needs to be updated.

If later patches in the series fix deficiencies in this patch (I haven't gotten past this description yet) then we should consider merging some of the patches together so they are more useful.

Quoted 6 lines

RGB: Explicitly declare prototypes as extern.
RGB: Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch, lsm_field.

Based-on-user-interface-by Richard Guy Briggs <[email protected]>

Based-on-idea-by Peter Moody <[email protected]>

I'm not fully up on the different patch metadata the cool kids are using these days, but I think it is okay to simply credit Peter in the patch description and omit the two lines above. Giving credit is important, but these metadata tags are a bit silly in my opinion.

Quoted 3 lines

Cc [email protected]

Signed-off-by Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

It has been a while since I've looked at Eric's original patch, but considering considering some of the work involved, I think it is reasonable to claim this patch as your own and credit Eric in the patch description.

Quoted 28 lines

kernel/audit.h +18 -0

index d641f9b..3aca24f 100644
@@ -278,6 +286,30 @@ 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_dupe_exe(struct audit_krule *new, struct
 audit_krule *old) +{
+	BUG();
+	return -EINVAL
+}

kernel/audit.h extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev · line 313

Quoted 6 lines

kernel/audit.h +4 -0

+static inline int audit_exe_compare(struct task_struct *tsk, struct
 audit_exe *exe) +{
+	BUG();
+	return 0;
+}
 #endif /* CONFIG_AUDIT_WATCH */

Not a big fan of the BUG() calls in the stubs above, let's get rid of them.

Yes, I know some other audit stubs are defined as BUG(), or similar, those aren't a good idea either, but they are already there ...

Quoted 64 lines

kernel/audit_exe.c new file +33 -0

new file mode 100644
index 0000000..d4cc8b5
@@ -0,0 +1,109 @@
+/* audit_exe.c -- filtering of audit events
+ *
+ * Copyright 2014-2015 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.
+ */
+
+#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)

kernel/audit_exe.c new file +24 -0

+{
+	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;

kernel/audit_exe.c line 61

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	exe->ino = ino;
+	exe->dev = dev;
+	exe->pathname = pathname;
+	krule->exe = exe;

You don't need the dev and ino variables here, just move the kmalloc() to just after the dentry->d_inode check ... or put it after the sanity checks, although you'll have to be careful to free it on error.

Quoted 8 lines

kernel/audit_exe.c new file +8 -0

+	return 0;
+}
+
+void audit_remove_exe_rule(struct audit_krule *krule)
+{
+	struct audit_exe *exe;
+
+	exe = krule->exe;

kernel/audit_exe.c line 74

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	krule->exe = NULL;
+	kfree(exe->pathname);
+	kfree(exe);
+}

Not your fault, and nothing to change here, but I really hate how audit dups strings outside the rule creation functions, but frees the strings in the rule free routines; it's almost asking to be misused.

Quoted 30 lines

kernel/audit_exe.c new file +30 -0

+char *audit_exe_path(struct audit_exe *exe)
+{
+	return exe->pathname;
+}
+
+int audit_dupe_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;

kernel/audit_exe.c line 109

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	if (tsk->mm->exe_file->f_inode->i_sb->s_dev != exe->dev)
+		return 0;
+	return 1;
+}

I suspect Eric put the above functions in a separate file to ease development (no need to work about messy porting as upstream moved on), but I see no reason why these functions couldn't be folded into auditfilter.c.

Quoted 36 lines

kernel/auditsc.c +13 -0

index 9fb9d1c..bf745c7 100644
@@ -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>

@@ -71,6 +72,7 @@
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
+#include <linux/sched.h>
 #include <linux/ctype.h>
 #include <linux/string.h>
 #include <uapi/linux/limits.h>

@@ -466,6 +468,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;
+				}

kernel/auditsc.c static int audit_filter_rules(struct task_struct *tsk, · line 485

Quoted 3 lines

kernel/auditsc.c +3 -0

+			}
+		}
+			break;

I don't completely understand the point of AUDIT_EXE_CHILDREN filter, what problem are we trying to solve? It checks to see if there is an executable match starting with the current process and walking up the process' parents in the current pid namespace?

Help me understand what this accomplishes, I'm a little tried right now and I just don't get it.

paul moore

Quoted 9 lines · Paul Moore

Quoted 7 lines · Richard Guy Briggs

From: Eric Paris <[email protected]>

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.

Quoted 3 lines

This needs work. Either this patch is incomplete as the description says, in which case I'm not going to merge it, or the description is out of date and needs to be updated.

It is pretty close to the original, so the description is valid, however combining this patch with 3/4 and 4/4 triggers a rewrite.

Quoted 3 lines

If later patches in the series fix deficiencies in this patch (I haven't gotten past this description yet) then we should consider merging some of the patches together so they are more useful.

Ok, no problem. (More below...)

Quoted 7 lines

Quoted 6 lines

RGB: Explicitly declare prototypes as extern.
RGB: Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch, lsm_field.

Based-on-user-interface-by Richard Guy Briggs <[email protected]>

Based-on-idea-by Peter Moody <[email protected]>

Quoted 4 lines

I'm not fully up on the different patch metadata the cool kids are using these days, but I think it is okay to simply credit Peter in the patch description and omit the two lines above. Giving credit is important, but these metadata tags are a bit silly in my opinion.

Fair enough. If it doesn't need to be machine-readable, I'll merge it into the patch description prose.

Quoted 35 lines

Quoted 35 lines

kernel/audit.h +19 -0

index d641f9b..3aca24f 100644
@@ -278,6 +286,30 @@ 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_dupe_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) +{

kernel/audit.h +3 -0

+	BUG();
+	return 0;
+}

Quoted 4 lines

Quoted 1 line

#endif /* CONFIG_AUDIT_WATCH */

Not a big fan of the BUG() calls in the stubs above, let's get rid of them.

Ok, that way I can more easily convert them to #defines.

Quoted 2 lines

Yes, I know some other audit stubs are defined as BUG(), or similar, those aren't a good idea either, but they are already there ...

Ok, cleanup patch later...

Quoted 64 lines

Quoted 64 lines

kernel/audit_exe.c new file +33 -0

new file mode 100644
index 0000000..d4cc8b5
@@ -0,0 +1,109 @@
+/* audit_exe.c -- filtering of audit events
+ *
+ * Copyright 2014-2015 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.
+ */
+
+#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)

kernel/audit_exe.c new file +24 -0

+{
+	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;

Quoted 8 lines

kernel/audit_exe.c line 61

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	exe->ino = ino;
+	exe->dev = dev;
+	exe->pathname = pathname;
+	krule->exe = exe;

You don't need the dev and ino variables here, just move the kmalloc() to just after the dentry->d_inode check ... or put it after the sanity checks, although you'll have to be careful to free it on error.

I'll take a closer look. As referenced elsewhere, I agree a helper function may be useful.

Quoted 8 lines

Quoted 8 lines

kernel/audit_exe.c new file +8 -0

+	return 0;
+}
+
+void audit_remove_exe_rule(struct audit_krule *krule)
+{
+	struct audit_exe *exe;
+
+	exe = krule->exe;

Quoted 8 lines

kernel/audit_exe.c line 74

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	krule->exe = NULL;
+	kfree(exe->pathname);
+	kfree(exe);
+}

Not your fault, and nothing to change here, but I really hate how audit dups strings outside the rule creation functions, but frees the strings in the rule free routines; it's almost asking to be misused.

Again, cleanup patch later maybe...

Quoted 31 lines

Quoted 31 lines

kernel/audit_exe.c new file +31 -0

+char *audit_exe_path(struct audit_exe *exe)
+{
+	return exe->pathname;
+}
+
+int audit_dupe_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)

Quoted 8 lines

kernel/audit_exe.c line 109

Quoted 3 lines

kernel/audit_exe.c new file +3 -0

+		return 0;
+	return 1;
+}

I suspect Eric put the above functions in a separate file to ease development (no need to work about messy porting as upstream moved on), but I see no reason why these functions couldn't be folded into auditfilter.c.

I thought it made sense where Eric put it. It somewhat parallelled the watch and tree code. It might be tempting to put it in audit_fsnotify.c, but I don't really want to overload that, since the fsnotify code may be used to simpify the watch code in the future. When we're done after 3/4, audit_exe.c is down to 50 lines...

Mind you, auditsc.c is a bit overloaded with stuff that doesn't necessarily belong there...

Quoted 46 lines

Quoted 46 lines

kernel/auditsc.c +1 -0

index 9fb9d1c..bf745c7 100644
@@ -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>

kernel/auditsc.c +1 -0

@@ -71,6 +72,7 @@
 
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
 
+#include <linux/sched.h>
 

#include <linux/ctype.h>
#include <linux/string.h>
#include <uapi/linux/limits.h>

kernel/auditsc.c +7 -0

@@ -466,6 +468,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)) {

Quoted 12 lines

kernel/auditsc.c static int audit_filter_rules(struct task_struct *tsk, · line 484

Quoted 7 lines

kernel/auditsc.c +7 -0

+				if (audit_exe_compare(ptsk, rule->exe)) {
+					++result;
+					break;
+				}
+			}
+		}
+			break;

I don't completely understand the point of AUDIT_EXE_CHILDREN filter, what problem are we trying to solve? It checks to see if there is an executable match starting with the current process and walking up the process' parents in the current pid namespace?

Say we want to monitor /usr/sbin/apache2 and all its spawned processes. Set up a rule that uses AUDIT_EXE_CHILDREN with /usr/sbin/apache2, then when it spawns a cgi running perl or php, those actions will be caught.

Quoted 2 lines

Help me understand what this accomplishes, I'm a little tried right now and I just don't get it.

This was Peter Moody's idea and it made sense, so we kept it.

Quoted 35 lines · Richard Guy Briggs

Quoted 9 lines · Paul Moore

Quoted 7 lines · Richard Guy Briggs

From: Eric Paris <[email protected]>

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.

Quoted 3 lines

This needs work. Either this patch is incomplete as the description says, in which case I'm not going to merge it, or the description is out of date and needs to be updated.

It is pretty close to the original, so the description is valid, however combining this patch with 3/4 and 4/4 triggers a rewrite.

Quoted 3 lines

If later patches in the series fix deficiencies in this patch (I haven't gotten past this description yet) then we should consider merging some of the patches together so they are more useful.

Ok, no problem. (More below...)

Quoted 11 lines

Quoted 6 lines

RGB: Explicitly declare prototypes as extern.
RGB: Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch, lsm_field.

Based-on-user-interface-by Richard Guy Briggs <[email protected]>

Based-on-idea-by Peter Moody <[email protected]>

I'm not fully up on the different patch metadata the cool kids are using these days, but I think it is okay to simply credit Peter in the patch description and omit the two lines above. Giving credit is important, but these metadata tags are a bit silly in my opinion.

Quoted 2 lines

Fair enough. If it doesn't need to be machine-readable, I'll merge it into the patch description prose.

You could do a "based on" or similar tag if you want. I'm honestly not sure what the official tags are beyond signed-off, acked, and reviewed. Those are the only ones I really care about anyway ;)

From Documentation/SubmittingPatches:

It isn't something I care enough about to reject a patch over, I figured you were going to need to do some respin work anyway so I wanted to mention it.

Quoted 40 lines

Quoted 35 lines

Quoted 35 lines

kernel/audit.h +19 -0

index d641f9b..3aca24f 100644
@@ -278,6 +286,30 @@ 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_dupe_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) +{

kernel/audit.h +3 -0

+	BUG();
+	return 0;
+}

Quoted 4 lines

Quoted 1 line

#endif /* CONFIG_AUDIT_WATCH */

Not a big fan of the BUG() calls in the stubs above, let's get rid of them.

Quoted 6 lines

Ok, that way I can more easily convert them to #defines.

Quoted 2 lines

Yes, I know some other audit stubs are defined as BUG(), or similar, those aren't a good idea either, but they are already there ...

Ok, cleanup patch later...

Much later. It isn't something I worry much about, I just don't want to see us going crazy with BUG assertions in new code.

Quoted 73 lines

Quoted 64 lines

Quoted 64 lines

kernel/audit_exe.c new file +33 -0

new file mode 100644
index 0000000..d4cc8b5
@@ -0,0 +1,109 @@
+/* audit_exe.c -- filtering of audit events
+ *
+ * Copyright 2014-2015 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.
+ */
+
+#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)

kernel/audit_exe.c new file +24 -0

+{
+	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;

Quoted 8 lines

kernel/audit_exe.c line 61

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	exe->ino = ino;
+	exe->dev = dev;
+	exe->pathname = pathname;
+	krule->exe = exe;

You don't need the dev and ino variables here, just move the kmalloc() to just after the dentry->d_inode check ... or put it after the sanity checks, although you'll have to be careful to free it on error.

Quoted 2 lines

I'll take a closer look. As referenced elsewhere, I agree a helper function may be useful.

Once again, not something I would worry about for this patchset, let's just get this code fixed up and merged. It was more of an observation that I see a lot of kernel audit structures storing and comparing dev/inode information and each way is slightly different ... I *love* consistency in code to an unhealthy level, so this bugs me a bit more than it probably should.

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

kernel/audit_exe.c new file +8 -0

+	return 0;
+}
+
+void audit_remove_exe_rule(struct audit_krule *krule)
+{
+	struct audit_exe *exe;
+
+	exe = krule->exe;

Quoted 8 lines

kernel/audit_exe.c line 74

Quoted 4 lines

kernel/audit_exe.c new file +4 -0

+	krule->exe = NULL;
+	kfree(exe->pathname);
+	kfree(exe);
+}

Not your fault, and nothing to change here, but I really hate how audit dups strings outside the rule creation functions, but frees the strings in the rule free routines; it's almost asking to be misused.

Quoted 1 line

Again, cleanup patch later maybe...

Yeah ... maybe. World peace too.

Quoted 40 lines

Quoted 31 lines

Quoted 31 lines

kernel/audit_exe.c new file +31 -0

+char *audit_exe_path(struct audit_exe *exe)
+{
+	return exe->pathname;
+}
+
+int audit_dupe_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)

Quoted 8 lines

kernel/audit_exe.c line 109

Quoted 3 lines

kernel/audit_exe.c new file +3 -0

+		return 0;
+	return 1;
+}

I suspect Eric put the above functions in a separate file to ease development (no need to work about messy porting as upstream moved on), but I see no reason why these functions couldn't be folded into auditfilter.c.

Quoted 5 lines

I thought it made sense where Eric put it. It somewhat parallelled the watch and tree code. It might be tempting to put it in audit_fsnotify.c, but I don't really want to overload that, since the fsnotify code may be used to simpify the watch code in the future. When we're done after 3/4, audit_exe.c is down to 50 lines...

See, there ya go ... I don't see the point of adding a new file for 50 lines.

Quoted 2 lines

Mind you, auditsc.c is a bit overloaded with stuff that doesn't necessarily belong there...

At some point we'll clean things up a bit - most likely as part of the upcoming audit rework - but it doesn't need to happen with this patchset.

Quoted 59 lines

Quoted 46 lines

Quoted 46 lines

kernel/auditsc.c +1 -0

index 9fb9d1c..bf745c7 100644
@@ -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>

kernel/auditsc.c +1 -0

@@ -71,6 +72,7 @@
 
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
 
+#include <linux/sched.h>
 

#include <linux/ctype.h>
#include <linux/string.h>
#include <uapi/linux/limits.h>

kernel/auditsc.c +7 -0

@@ -466,6 +468,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)) {

Quoted 12 lines

kernel/auditsc.c static int audit_filter_rules(struct task_struct *tsk, · line 484

Quoted 7 lines

kernel/auditsc.c +7 -0

+				if (audit_exe_compare(ptsk, rule->exe)) {
+					++result;
+					break;
+				}
+			}
+		}
+			break;

I don't completely understand the point of AUDIT_EXE_CHILDREN filter, what problem are we trying to solve? It checks to see if there is an executable match starting with the current process and walking up the process' parents in the current pid namespace?

Quoted 8 lines

Say we want to monitor /usr/sbin/apache2 and all its spawned processes. Set up a rule that uses AUDIT_EXE_CHILDREN with /usr/sbin/apache2, then when it spawns a cgi running perl or php, those actions will be caught.

Quoted 2 lines

Help me understand what this accomplishes, I'm a little tried right now and I just don't get it.

This was Peter Moody's idea and it made sense, so we kept it.

I suppose my confusion is that I see the exe filtering as very similar to our PID filtering, the big difference is that we allow you to match on an on-disk binary and not a running process. We don't currently have a PID_CHILDREN filter and presumably no one has asked for it. Yes we do have PPID, but it only goes up one level, e.g. it's PPID and not PPPPPPPPID, not like the potentially nasty loop we've got with EXE_CHILDREN.

I'm not super excited about the loop so I'd really like to hear how this is solving an actual user need and not just a "hey, wouldn't it be cool?" feature.

Signature · 7 lines

paul moore
security @ redhat

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 3 lines · Paul Moore

You could do a "based on" or similar tag if you want. I'm honestly not sure what the official tags are beyond signed-off, acked, and reviewed. Those are the only ones I really care about anyway ;)

From Documentation/SubmittingPatches:
11) Signed-off-by:
12) Acked-by: and Cc:
13) Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:

Quoted 8 lines · Paul Moore

Quoted 7 lines · Richard Guy Briggs

From: Eric Paris <[email protected]>

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.

Quoted 28 lines

Quoted 28 lines

kernel/auditsc.c +12 -0

index 9fb9d1c..bf745c7 100644
@@ -71,6 +72,7 @@
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
+#include <linux/sched.h>
 #include <linux/ctype.h>
 #include <linux/string.h>
 #include <uapi/linux/limits.h>

@@ -466,6 +468,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;
+				}

Quoted 8 lines

kernel/auditsc.c static int audit_filter_rules(struct task_struct *tsk, · line 485

Quoted 3 lines

kernel/auditsc.c +3 -0

+			}
+		}
+			break;

I don't completely understand the point of AUDIT_EXE_CHILDREN filter, what problem are we trying to solve? It checks to see if there is an executable match starting with the current process and walking up the process' parents in the current pid namespace?

Say we want to monitor /usr/sbin/apache2 and all its spawned processes. Set up a rule that uses AUDIT_EXE_CHILDREN with /usr/sbin/apache2, then when it spawns a cgi running perl or php, those actions will be caught.

Quoted 2 lines

Help me understand what this accomplishes, I'm a little tried right now and I just don't get it.

This was Peter Moody's idea and it made sense, so we kept it.

Quoted 1 line

paul moore

- RGB

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 optimization is necessary.

Signed-off-by Eric Paris <[email protected]>

RGB: Move the access to the entry for audit_match_signal() to the beginning of the function in case the entry found is the same one passed in. This will enable it to be used by audit_autoremove_mark_rule().
RGB: Rename several "watch" references to "mark".
RGB: Rename audit_remove_rule() to audit_autoremove_mark_rule().
RGB: Put audit_alloc_mark() arguments in same order as watch, tree and inode. RGB: Remove space from audit log value text in audit_autoremove_mark_rule(). RGB: Explicitly declare prototypes as extern.
RGB: Rename audit_remove_mark_rule() called from audit_mark_handle_event() to audit_autoremove_mark_rule() to avoid confusion with audit_remove_{watch,tree}_rule() usage.
RGB: Add audit_remove_mark_rule() to provide similar interface as

audit_remove_{watch,tree}_rule().
RGB: Simplify stubs to defines.
RGB: Rename audit_free_fsnotify_mark() to audit_fsnotify_free_mark() in keeping with the naming convention of inotify_free_mark(), dnotify_free_mark(), fanotify_free_mark(), audit_watch_free_mark().
RGB: Return -ENOMEM rather than null in case of memory allocation failure for audit_mark. RGB: Rename audit_free_mark() to audit_mark_free() to avoid association with {i,d,fa}notify_free_mark() and audit_watch_free_mark().

Based-on-code-by Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

kernel/Makefile +1 -1

index a7ea330..397109e 100644
@@ -64,7 +64,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

kernel/audit.h +0 -0

index 3aca24f..491bd4b 100644
@@ -286,6 +294,20 @@ extern 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

kernel/audit.h +9 -0

+#define audit_alloc_mark(k, p, l) (ERR_PTR(-EINVAL))
+static inline char *audit_mark_path(struct audit_fsnotify_mark *mark)
+{
+	BUG();
+	return "";
+}
+#define audit_remove_mark(m) BUG()
+#define audit_remove_mark_rule(k) BUG()
+static inline int audit_mark_compare(struct audit_fsnotify_mark *mark,

unsigned long ino, dev_t dev) +{

kernel/audit_fsnotify.c new file +16 -0

new file mode 100644
index 0000000..a4e7b16
@@ -0,0 +1,253 @@
+/* audit_fsnotify.c -- tracking inodes
+ *
+ * Copyright 2003-2009,2014-2015 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.
+ */

+struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len) +{

kernel/audit_fsnotify.c new file +15 -0

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

kernel/audit_fsnotify.c new file +15 -0

+	audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
+	if (unlikely(!audit_mark)) {
+		kfree(local_pathname);
+		audit_mark = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+
+	fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_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) {

kernel/audit_fsnotify.c new file +8 -0

+		audit_mark_free(audit_mark);
+		audit_mark = ERR_PTR(ret);
+	}
+out:
+	dput(dentry);
+	path_put(&path);
+	return audit_mark;
+}

+static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, char *op)
+{

kernel/audit_fsnotify.c new file +4 -0

+/* Update mark data in audit rules based on fsnotify events. */
+static int audit_mark_handle_event(struct fsnotify_group *group,
+				    struct inode *to_tell,
+				    struct fsnotify_mark *inode_mark,

kernel/auditfilter.c +2 -1

index 09041b2..bbb39ec 100644
@@ -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_tree *tree = entry->rule.tree;

@@ -985,6 +985,7 @@ static inline int audit_del_rule(struct audit_entry
 *entry) int ret = 0;
 #ifdef CONFIG_AUDITSYSCALL
 	int dont_count = 0;
+	int match = audit_match_signal(entry);
 
 	/* If either of these, don't count towards total */
 	if (entry->rule.listnr == AUDIT_FILTER_USER ||

Quoted 31 lines · Richard Guy Briggs

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 optimization is necessary.

Signed-off-by Eric Paris <[email protected]>

RGB: Move the access to the entry for audit_match_signal() to the beginning of the function in case the entry found is the same one passed in. This will enable it to be used by audit_autoremove_mark_rule().
RGB: Rename several "watch" references to "mark".
RGB: Rename audit_remove_rule() to audit_autoremove_mark_rule().
RGB: Put audit_alloc_mark() arguments in same order as watch, tree and inode. RGB: Remove space from audit log value text in audit_autoremove_mark_rule(). RGB: Explicitly declare prototypes as extern.
RGB: Rename audit_remove_mark_rule() called from audit_mark_handle_event() to audit_autoremove_mark_rule() to avoid confusion with audit_remove_{watch,tree}_rule() usage.
RGB: Add audit_remove_mark_rule() to provide similar interface as

Quoted 8 lines

audit_remove_{watch,tree}_rule().
RGB: Simplify stubs to defines.
RGB: Rename audit_free_fsnotify_mark() to audit_fsnotify_free_mark() in keeping with the naming convention of inotify_free_mark(), dnotify_free_mark(), fanotify_free_mark(), audit_watch_free_mark().
RGB: Return -ENOMEM rather than null in case of memory allocation failure for audit_mark. RGB: Rename audit_free_mark() to audit_mark_free() to avoid association with {i,d,fa}notify_free_mark() and audit_watch_free_mark().

Definitely enough changes here to call this your own; credit Eric in the description and just stick with your sign off.

Quoted 2 lines

Based-on-code-by Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

Based on the patch description, should this be patch 1/4 instead of 2/4?

Quoted 21 lines

kernel/Makefile +1 -1

index a7ea330..397109e 100644
@@ -64,7 +64,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

kernel/audit.h +0 -0

index 3aca24f..491bd4b 100644
@@ -286,6 +294,20 @@ extern 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

kernel/audit.h extern int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe); · line 309

Quoted 13 lines

kernel/audit.h +9 -0

+#define audit_alloc_mark(k, p, l) (ERR_PTR(-EINVAL))
+static inline char *audit_mark_path(struct audit_fsnotify_mark *mark)
+{
+	BUG();
+	return "";
+}
+#define audit_remove_mark(m) BUG()
+#define audit_remove_mark_rule(k) BUG()
+static inline int audit_mark_compare(struct audit_fsnotify_mark *mark,

unsigned long ino, dev_t dev) +{

kernel/audit.h +3 -0

+	BUG();
+	return 0;
+}

No BUG(), we've got enough of those things already.

Quoted 22 lines

kernel/audit_fsnotify.c new file +16 -0

new file mode 100644
index 0000000..a4e7b16
@@ -0,0 +1,253 @@
+/* audit_fsnotify.c -- tracking inodes
+ *
+ * Copyright 2003-2009,2014-2015 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.
+ */

...

kernel/audit_fsnotify.c line 62

Quoted 7 lines

kernel/audit_fsnotify.c new file +7 -0

+static void audit_fsnotify_free_mark(struct fsnotify_mark *mark)
+{
+	struct audit_fsnotify_mark *audit_mark;
+
+	audit_mark = container_of(mark, struct audit_fsnotify_mark, mark);
+	audit_mark_free(audit_mark);
+}

It seems like audit_fsnotify_mark_free() might be more consistent with the rest of the naming, yes?

Quoted 8 lines

kernel/audit_fsnotify.c new file +8 -0

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

kernel/audit_fsnotify.c line 76

Quoted 5 lines

kernel/audit_fsnotify.c new file +5 -0

+{
+	if (likely(audit_mark))
+		fsnotify_put_mark(&audit_mark->mark);
+}
+#endif

If this code is '#if 0' let's dump it. We need it or we don't, keeping it around as dead weight is dangerous.

Quoted 7 lines

kernel/audit_fsnotify.c new file +6 -0

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

kernel/audit_fsnotify.c line 88

Quoted 4 lines

kernel/audit_fsnotify.c new file +4 -0

+	if (mark->ino == (unsigned long)-1)
+		return 0;
+	return (mark->ino == ino) && (mark->dev == dev);
+}

It seems like there should be a #define for -1 inodes, did you check? I generally hate magic numbers like this because I'm pretty stupid and tend to forget their meaning over time ....

Also, at some point we should make (or find?) some generic inode comparison function/macro. I'm amazed at home many times I see (i_foo == ino && i_dev == dev).

Quoted 17 lines

+struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len) +{

kernel/audit_fsnotify.c new file +15 -0

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

kernel/audit_fsnotify.c line 116

Quoted 10 lines

kernel/audit_fsnotify.c new file +10 -0

+	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;
+	}

My comments on the ino and dev variables from the other patch apply here.

kernel/audit_fsnotify.c line 121

Quoted 4 lines

kernel/audit_fsnotify.c new file +4 -0

+	audit_mark = ERR_PTR(-ENOMEM);
+	local_pathname = kstrdup(pathname, GFP_KERNEL);
+	if (!local_pathname)
+		goto out;

Why not just kstrdup() into audit_mark->path directly? I don't get the fascination with local variables. Also, why bother with the strdup if the audit_mark malloc is going to fail?

Quoted 24 lines

kernel/audit_fsnotify.c new file +15 -0

+	audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
+	if (unlikely(!audit_mark)) {
+		kfree(local_pathname);
+		audit_mark = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+
+	fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_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) {

kernel/audit_fsnotify.c new file +8 -0

+		audit_mark_free(audit_mark);
+		audit_mark = ERR_PTR(ret);
+	}
+out:
+	dput(dentry);
+	path_put(&path);
+	return audit_mark;
+}

...

Quoted 3 lines

+static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, char *op)
+{

That is a lot of letters ... who about audit_mark_log_change()?

Quoted 11 lines

kernel/audit_fsnotify.c new file +11 -0

+/* Update mark data in audit rules based on fsnotify events. */
+static int audit_mark_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;
+

kernel/audit_fsnotify.c line 216

Quoted 3 lines

kernel/audit_fsnotify.c new file +3 -0

+	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
+
+	BUG_ON(group != audit_fsnotify_group);

What happens when BUG_ON() is compiled out? Let's back this up with some normal error checking if you think this is a real concern. If it isn't a real concern, why do we have a BUG_ON()? This doesn't strike me as something that is going to be a real problem.

Quoted 6 lines

kernel/audit_fsnotify.c new file +6 -0

+	switch (data_type) {
+	case (FSNOTIFY_EVENT_PATH):
+		inode = ((struct path *)data)->dentry->d_inode;
+		break;
+	case (FSNOTIFY_EVENT_INODE):
+		inode = (struct inode *)data;

kernel/audit_fsnotify.c line 228

Quoted 5 lines

kernel/audit_fsnotify.c new file +5 -0

+		break;
+	default:
+		BUG();
+		return 0;
+	};

We can do better than BUG() in the default catch-all above. Maybe a prink(KERN_ERR ...)?

Quoted 25 lines

kernel/auditfilter.c +2 -1

index 09041b2..bbb39ec 100644
@@ -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_tree *tree = entry->rule.tree;

@@ -985,6 +985,7 @@ static inline int audit_del_rule(struct audit_entry
 *entry) int ret = 0;
 #ifdef CONFIG_AUDITSYSCALL
 	int dont_count = 0;
+	int match = audit_match_signal(entry);
 
 	/* If either of these, don't count towards total */
 	if (entry->rule.listnr == AUDIT_FILTER_USER ||

@@ -1017,7 +1018,7 @@ static inline int audit_del_rule(struct audit_entry
 *entry) if (!dont_count)
 		audit_n_rules--;
 

kernel/auditfilter.c static inline int audit_del_rule(struct audit_entry *entry) · line 1024

Quoted 5 lines

kernel/auditfilter.c +1 -1

-	if (!audit_match_signal(entry))
+	if (!match)
 		audit_signals--;
 #endif
 	mutex_unlock(&audit_filter_mutex);

Is the bit above worthy of it's own bugfix patch independent of this fsnotify implementation, or is it only an issue with this new fsnotify code?

paul moore

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 <[email protected]>

RGB: Clean up exe with similar interface as watch and tree.
RGB: Clean up audit exe mark just before audit_free_rule() rather than in it to avoid mutex in software interrupt context.

Based-on-code-by Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

include/linux/audit.h +1 -1

index 95463a2..aee456f 100644
@@ -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;

kernel/audit.h +0 -1

index 491bd4b..eeaf054 100644
@@ -51,7 +51,6 @@ enum audit_state {
 /* Rule lists */
 struct audit_watch;
 struct audit_fsnotify_mark;
-struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
 

@@ -279,11 +278,8 @@ extern void audit_remove_mark(struct

audit_fsnotify_mark *audit_mark); extern void audit_remove_mark_rule(struct audit_krule *krule);
extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned
long ino, dev_t dev);

-extern int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op); -extern void audit_remove_exe_rule(struct audit_krule *krule);
-extern char *audit_exe_path(struct audit_exe *exe);
extern int audit_dupe_exe(struct audit_krule *new, struct audit_krule
*old); -extern int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe); +extern int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark);

#else
#define audit_put_watch(w) {}

kernel/audit.h +5 -19

@@ -302,36 +298,19 @@ static inline char *audit_mark_path(struct
 audit_fsnotify_mark *mark) }
 #define audit_remove_mark(m) BUG()
 #define audit_remove_mark_rule(k) 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;
-}
-static inline void audit_remove_exe_rule(struct audit_krule *krule)
-{
-	BUG();
-	return 0;
-}
-static inline char *audit_exe_path(struct audit_exe *exe)
+static inline int audit_exe_compare(struct task_struct *tsk, struct
 audit_fsnotify_mark *mark) {
 	BUG();
-	return "";
+	return -EINVAL;
 }
+
 static inline int audit_dupe_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;
+	return -EINVAL;
 }
+
 #endif /* CONFIG_AUDIT_WATCH */
 
 #ifdef CONFIG_AUDIT_TREE

kernel/audit_exe.c +12 -74

index d4cc8b5..75ad4f2 100644
@@ -17,93 +17,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_dupe_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(new, pathname, strlen(pathname));
+	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);
 }

kernel/audit_tree.c +2 -0

index b0f9877..94ecdab 100644
@@ -479,6 +479,8 @@ static void kill_rules(struct audit_tree *tree)
 		if (rule->tree) {
 			/* not a half-baked one */
 			audit_tree_log_remove_rule(rule);
+			if (entry->rule.exe)
+				audit_remove_mark(entry->rule.exe);
 			rule->tree = NULL;
 			list_del_rcu(&entry->list);
 			list_del(&entry->rule.list);

kernel/audit_watch.c +4 -0

index 8f123d7..4aaa393 100644
@@ -312,6 +312,8 @@ static void audit_update_watch(struct audit_parent
 *parent, list_replace(&oentry->rule.list,
 					     &nentry->rule.list);
 			}
+			if (oentry->rule.exe)
+				audit_remove_mark(oentry->rule.exe);
 
 			audit_watch_log_rule_change(r, owatch, "updated_rules");
 

@@ -342,6 +344,8 @@ static void audit_remove_parent_watches(struct
 audit_parent *parent) list_for_each_entry_safe(r, nextr, &w->rules, rlist)
 {
 			e = container_of(r, struct audit_entry, rule);
 			audit_watch_log_rule_change(r, w, "remove_rule");
+			if (e->rule.exe)
+				audit_remove_mark(e->rule.exe);
 			list_del(&r->rlist);
 			list_del(&r->list);

kernel/audit_watch.c +0 -0

 			list_del_rcu(&e->list);

kernel/auditfilter.c +0 -0

index bbb39ec..f65c97f 100644
@@ -426,6 +426,7 @@ static struct audit_entry *audit_data_to_entry(struct

audit_rule_data *data, size_t remain = datasz - sizeof(struct audit_rule_data);

kernel/auditfilter.c +9 -4

 	int i;
 	char *str;
+	struct audit_fsnotify_mark *audit_mark;
 
 	entry = audit_to_entry_common(data);
 	if (IS_ERR(entry))

@@ -557,11 +558,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(&entry->rule, str, f->val);
+			kfree(str);
+			if (IS_ERR(audit_mark)) {
+				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}
+			entry->rule.exe = audit_mark;
 			break;
 		}
 	}

@@ -575,6 +578,8 @@ exit_nofree:
 exit_free:
 	if (entry->rule.tree)
 		audit_put_tree(entry->rule.tree); /* that's the temporary one */
+	if (entry->rule.exe)
+		audit_remove_mark(entry->rule.exe); /* that's the template one */
 	audit_free_rule(entry);
 	return ERR_PTR(err);
 }

@@ -642,7 +647,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;
 		case AUDIT_LOGINUID_SET:
 			if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {

@@ -710,8 +715,8 @@ static int audit_compare_rule(struct audit_krule *a,

struct audit_krule *b) case AUDIT_EXE:

kernel/auditfilter.c +11 -4

 		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:

@@ -842,6 +847,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
 *old) break;
 		}
 		if (err) {
+			if (new->exe)
+				audit_remove_mark(new->exe);
 			audit_free_rule(entry);
 			return ERR_PTR(err);
 		}

@@ -1008,7 +1015,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_rule(&e->rule);
 
 	list_del_rcu(&e->list);
 	list_del(&e->rule.list);

@@ -1113,8 +1120,11 @@ int audit_rule_change(int type, __u32 portid, int
 seq, void *data, WARN_ON(1);
 	}
 
-	if (err || type == AUDIT_DEL_RULE)
+	if (err || type == AUDIT_DEL_RULE) {
+		if (entry->rule.exe)
+			audit_remove_mark(entry->rule.exe);
 		audit_free_rule(entry);
+	}
 
 	return err;
 }

@@ -1406,6 +1416,8 @@ static int update_lsm_rule(struct audit_krule *r)
 		return 0;
 
 	nentry = audit_dupe_rule(r);
+	if (entry->rule.exe)
+		audit_remove_mark(entry->rule.exe);
 	if (IS_ERR(nentry)) {
 		/* save the first error encountered for the
 		 * return value */

Quoted 12 lines · Richard Guy Briggs

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 <[email protected]>

RGB: Clean up exe with similar interface as watch and tree.
RGB: Clean up audit exe mark just before audit_free_rule() rather than in it to avoid mutex in software interrupt context.

Quoted 2 lines

Based-on-code-by Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

Ah, good, the fix that patch 1/4 was hoping for is finally happening :)

Since we're not getting paid by the number of patches in a patch set, I think I would prefer to see the fsnotify implementation as the first patch and the original crappy exe filter patch merged with this fixup patch.

No in depth comments on this particular patch, I skimmed it but frankly it makes much more sense to review this patch once you've merged the two.

Quoted 323 lines

include/linux/audit.h +1 -1

index 95463a2..aee456f 100644
@@ -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;

kernel/audit.h +0 -1

index 491bd4b..eeaf054 100644
@@ -51,7 +51,6 @@ enum audit_state {
 /* Rule lists */
 struct audit_watch;
 struct audit_fsnotify_mark;
-struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
 

@@ -279,11 +278,8 @@ extern void audit_remove_mark(struct

audit_fsnotify_mark *audit_mark); extern void audit_remove_mark_rule(struct audit_krule *krule);
extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned
long ino, dev_t dev);

-extern int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op); -extern void audit_remove_exe_rule(struct audit_krule *krule);
-extern char *audit_exe_path(struct audit_exe *exe);
extern int audit_dupe_exe(struct audit_krule *new, struct audit_krule
*old); -extern int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe); +extern int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark);

#else
#define audit_put_watch(w) {}

kernel/audit.h +5 -19

@@ -302,36 +298,19 @@ static inline char *audit_mark_path(struct
 audit_fsnotify_mark *mark) }
 #define audit_remove_mark(m) BUG()
 #define audit_remove_mark_rule(k) 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;
-}
-static inline void audit_remove_exe_rule(struct audit_krule *krule)
-{
-	BUG();
-	return 0;
-}
-static inline char *audit_exe_path(struct audit_exe *exe)
+static inline int audit_exe_compare(struct task_struct *tsk, struct
 audit_fsnotify_mark *mark) {
 	BUG();
-	return "";
+	return -EINVAL;
 }
+
 static inline int audit_dupe_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;
+	return -EINVAL;
 }
+
 #endif /* CONFIG_AUDIT_WATCH */
 
 #ifdef CONFIG_AUDIT_TREE

kernel/audit_exe.c +12 -74

index d4cc8b5..75ad4f2 100644
@@ -17,93 +17,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_dupe_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(new, pathname, strlen(pathname));
+	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);
 }

kernel/audit_tree.c +2 -0

index b0f9877..94ecdab 100644
@@ -479,6 +479,8 @@ static void kill_rules(struct audit_tree *tree)
 		if (rule->tree) {
 			/* not a half-baked one */
 			audit_tree_log_remove_rule(rule);
+			if (entry->rule.exe)
+				audit_remove_mark(entry->rule.exe);
 			rule->tree = NULL;
 			list_del_rcu(&entry->list);
 			list_del(&entry->rule.list);

kernel/audit_watch.c +4 -0

index 8f123d7..4aaa393 100644
@@ -312,6 +312,8 @@ static void audit_update_watch(struct audit_parent
 *parent, list_replace(&oentry->rule.list,
 					     &nentry->rule.list);
 			}
+			if (oentry->rule.exe)
+				audit_remove_mark(oentry->rule.exe);
 
 			audit_watch_log_rule_change(r, owatch, "updated_rules");
 

@@ -342,6 +344,8 @@ static void audit_remove_parent_watches(struct
 audit_parent *parent) list_for_each_entry_safe(r, nextr, &w->rules, rlist)
 {
 			e = container_of(r, struct audit_entry, rule);
 			audit_watch_log_rule_change(r, w, "remove_rule");
+			if (e->rule.exe)
+				audit_remove_mark(e->rule.exe);
 			list_del(&r->rlist);
 			list_del(&r->list);

kernel/audit_watch.c +0 -0

 			list_del_rcu(&e->list);

kernel/auditfilter.c +0 -0

index bbb39ec..f65c97f 100644
@@ -426,6 +426,7 @@ static struct audit_entry *audit_data_to_entry(struct

audit_rule_data *data, size_t remain = datasz - sizeof(struct audit_rule_data);

kernel/auditfilter.c +9 -4

 	int i;
 	char *str;
+	struct audit_fsnotify_mark *audit_mark;
 
 	entry = audit_to_entry_common(data);
 	if (IS_ERR(entry))

@@ -557,11 +558,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(&entry->rule, str, f->val);
+			kfree(str);
+			if (IS_ERR(audit_mark)) {
+				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}
+			entry->rule.exe = audit_mark;
 			break;
 		}
 	}

@@ -575,6 +578,8 @@ exit_nofree:
 exit_free:
 	if (entry->rule.tree)
 		audit_put_tree(entry->rule.tree); /* that's the temporary one */
+	if (entry->rule.exe)
+		audit_remove_mark(entry->rule.exe); /* that's the template one */
 	audit_free_rule(entry);
 	return ERR_PTR(err);
 }

@@ -642,7 +647,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;
 		case AUDIT_LOGINUID_SET:
 			if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {

@@ -710,8 +715,8 @@ static int audit_compare_rule(struct audit_krule *a,

struct audit_krule *b) case AUDIT_EXE:

kernel/auditfilter.c +9 -4

 		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:

@@ -842,6 +847,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
 *old) break;
 		}
 		if (err) {
+			if (new->exe)
+				audit_remove_mark(new->exe);
 			audit_free_rule(entry);
 			return ERR_PTR(err);
 		}

@@ -1008,7 +1015,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_rule(&e->rule);
 
 	list_del_rcu(&e->list);
 	list_del(&e->rule.list);

@@ -1113,8 +1120,11 @@ int audit_rule_change(int type, __u32 portid, int
 seq, void *data, WARN_ON(1);
 	}
 
-	if (err || type == AUDIT_DEL_RULE)
+	if (err || type == AUDIT_DEL_RULE) {
+		if (entry->rule.exe)
+			audit_remove_mark(entry->rule.exe);
 		audit_free_rule(entry);
+	}
 
 	return err;
 }

@@ -1406,6 +1416,8 @@ static int update_lsm_rule(struct audit_krule *r)
 		return 0;
 

Quoted 6 lines

kernel/auditfilter.c +2 -0

 	nentry = audit_dupe_rule(r);
+	if (entry->rule.exe)
+		audit_remove_mark(entry->rule.exe);
 	if (IS_ERR(nentry)) {
 		/* save the first error encountered for the
 		 * return value */

Signature · 2 lines

paul moore
security @ redhat

Make this interface consistent with watch and filter key, avoiding the extra string copy and simply consume the new string pointer.

Signed-off-by Richard Guy Briggs <[email protected]>

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

kernel/audit_exe.c +6 -2

index 75ad4f2..09e4eb4 100644
@@ -27,11 +27,15 @@ int audit_dupe_exe(struct audit_krule *new, struct
 audit_krule *old) struct audit_fsnotify_mark *audit_mark;
 	char *pathname;
 
-	pathname = audit_mark_path(old->exe);
+	pathname = kstrdup(audit_mark_path(old->exe), GFP_KERNEL);
+	if (!pathname)
+		return -ENOMEM;
 
 	audit_mark = audit_alloc_mark(new, pathname, strlen(pathname));
-	if (IS_ERR(audit_mark))
+	if (IS_ERR(audit_mark)) {
+		kfree(pathname);
 		return PTR_ERR(audit_mark);
+	}
 	new->exe = audit_mark;
 
 	return 0;

kernel/audit_fsnotify.c +1 -8

index a4e7b16..e57e08a 100644
@@ -94,7 +94,6 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct
 audit_krule *krule, char *pa struct dentry *dentry;
 	struct inode *inode;
 	unsigned long ino;
-	char *local_pathname;
 	dev_t dev;
 	int ret;
 

@@ -115,21 +114,15 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct
 audit_krule *krule, char *pa 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);
 		audit_mark = ERR_PTR(-ENOMEM);
 		goto out;
 	}
 
 	fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_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;

kernel/auditfilter.c +1 -1

index f65c97f..f46ed69 100644
@@ -559,8 +559,8 @@ static struct audit_entry *audit_data_to_entry(struct
 audit_rule_data *data, entry->rule.buflen += f->val;
 
 			audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
-			kfree(str);
 			if (IS_ERR(audit_mark)) {
+				kfree(str);
 				err = PTR_ERR(audit_mark);
 				goto exit_free;
 			}

Quoted 3 lines · Richard Guy Briggs

Make this interface consistent with watch and filter key, avoiding the extra string copy and simply consume the new string pointer.

Quoted 6 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

Merge this patch too, there is no reason why these needs to be its own patch.

Quoted 64 lines

kernel/audit_exe.c +6 -2

index 75ad4f2..09e4eb4 100644
@@ -27,11 +27,15 @@ int audit_dupe_exe(struct audit_krule *new, struct
 audit_krule *old) struct audit_fsnotify_mark *audit_mark;
 	char *pathname;
 
-	pathname = audit_mark_path(old->exe);
+	pathname = kstrdup(audit_mark_path(old->exe), GFP_KERNEL);
+	if (!pathname)
+		return -ENOMEM;
 
 	audit_mark = audit_alloc_mark(new, pathname, strlen(pathname));
-	if (IS_ERR(audit_mark))
+	if (IS_ERR(audit_mark)) {
+		kfree(pathname);
 		return PTR_ERR(audit_mark);
+	}
 	new->exe = audit_mark;
 
 	return 0;

kernel/audit_fsnotify.c +1 -8

index a4e7b16..e57e08a 100644
@@ -94,7 +94,6 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct
 audit_krule *krule, char *pa struct dentry *dentry;
 	struct inode *inode;
 	unsigned long ino;
-	char *local_pathname;
 	dev_t dev;
 	int ret;
 

@@ -115,21 +114,15 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct
 audit_krule *krule, char *pa 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);
 		audit_mark = ERR_PTR(-ENOMEM);
 		goto out;
 	}
 
 	fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_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;

kernel/auditfilter.c +0 -0

index f65c97f..f46ed69 100644
@@ -559,8 +559,8 @@ static struct audit_entry *audit_data_to_entry(struct
 audit_rule_data *data, entry->rule.buflen += f->val;