Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1183811 > unrolled thread

[PATCH V6 0/4] audit by executable name

Started byRichard Guy Briggs <rgb@redhat.com>
First post2015-07-14 18:00 +0200
Last post2015-07-15 14:30 +0200
Articles 10 — 3 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH V6 0/4] audit by executable name Richard Guy Briggs <rgb@redhat.com> - 2015-07-14 18:00 +0200
    [PATCH V6 1/4] audit: implement audit by executable Richard Guy Briggs <rgb@redhat.com> - 2015-07-14 18:00 +0200
      Re: [PATCH V6 1/4] audit: implement audit by executable Paul Moore <pmoore@redhat.com> - 2015-07-17 03:20 +0200
    [PATCH V6 4/4] audit: avoid double copying the audit_exe path string Richard Guy Briggs <rgb@redhat.com> - 2015-07-14 18:00 +0200
      Re: [PATCH V6 4/4] audit: avoid double copying the audit_exe path string Paul Moore <pmoore@redhat.com> - 2015-07-17 04:00 +0200
    [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify Richard Guy Briggs <rgb@redhat.com> - 2015-07-14 18:00 +0200
      Re: [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify Paul Moore <pmoore@redhat.com> - 2015-07-17 04:00 +0200
    [PATCH V6 2/4] audit: clean simple fsnotify implementation Richard Guy Briggs <rgb@redhat.com> - 2015-07-14 18:00 +0200
      Re: [PATCH V6 2/4] audit: clean simple fsnotify implementation Paul Moore <pmoore@redhat.com> - 2015-07-17 03:50 +0200
    Re: [PATCH V6 0/4] audit by executable name Steve Grubb <sgrubb@redhat.com> - 2015-07-15 14:30 +0200

#1183811 — [PATCH V6 0/4] audit by executable name

FromRichard Guy Briggs <rgb@redhat.com>
Date2015-07-14 18:00 +0200
Subject[PATCH V6 0/4] audit by executable name
Message-ID<pMaMG-4Md-7@gated-at.bofh.it>
Please see the accompanying userspace patchset:
	https://www.redhat.com/archives/linux-audit/2015-July/thread.html
	[[PATCH V2] 0/2] Log on the future execution of a path
The userspace interface is not expected to change appreciably unless something
important has been overlooked.  Setting and deleting rules works as expected.
	
If the path does not exist at rule creation time, it will be re-evaluated every
time there is a change to the parent directory at which point the change in
device and inode will be noted.


Here's a sample run:
Test for addition, trigger and deletion of tree executable rule:
# auditctl -a always,exit -S all -F dir=/tmp -F exe=/usr/bin/touch -F key=exetest_tree
----
time->Sat Jul 11 10:41:50 2015
type=CONFIG_CHANGE msg=audit(1436629310.720:44711): auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="add_rule" key="exetest_tree" list=4 res=1
----

# /usr/bin/touch /tmp/test
----
time->Sat Jul 11 10:41:50 2015
type=PROCTITLE msg=audit(1436629310.757:44712): proctitle=2F7573722F62696E2F746F756368002F746D702F74657374
type=PATH msg=audit(1436629310.757:44712): item=1 name="/tmp/test" inode=166932 dev=00:24 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE
type=PATH msg=audit(1436629310.757:44712): item=0 name="/tmp/" inode=11525 dev=00:24 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT
type=CWD msg=audit(1436629310.757:44712):  cwd="/root"
type=SYSCALL msg=audit(1436629310.757:44712): arch=c000003e syscall=2 success=yes exit=3 a0=7ffdee2f9e27 a1=941 a2=1b6 a3=691 items=2 ppid=17655 pid=17762 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=ttyS0 ses=1 comm="touch" exe="/usr/bin/touch" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="exetest_tree"
----

# auditctl -d always,exit -S all -F dir=/tmp -F exe=/usr/bin/touch -F key=exetest_tree
----
time->Sat Jul 11 10:41:50 2015
type=CONFIG_CHANGE msg=audit(1436629310.839:44713): auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="remove_rule" key="exetest_tree" list=4 res=1
----


Revision history:
v6: Explicitly declare prototypes as external.
    Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch, lsm_field.
    Rebased on v4.1.
    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.
    Add audit_remove_mark_rule() to provide similar interface as
      audit_remove_{watch,tree}_rule().
    Simplify stubs to defines.
    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().
    Return -ENOMEM rather than null in case of memory allocation failure for
      audit_mark in audit_alloc_mark().
    Rename audit_free_mark() to audit_mark_free() to avoid association with
      {i,d,fa}notify_free_mark() and audit_watch_free_mark().
    Clean up exe with similar interface as watch and tree.
    Clean up audit exe mark just before audit_free_rule() rather than in it to
      avoid mutex in software interrupt context.
    Fixed bug in audit_dupe_exe() that returned error rather than valid pointer.

v5: Revert patch "Let audit_free_rule() take care of calling
    audit_remove_mark()." since it caused a group mark deadlock.
    https://www.redhat.com/archives/linux-audit/2014-October/msg00024.html

v4: Re-order and squash down fixups
    Fix audit_dup_exe() to copy pathname string before calling audit_alloc_mark().
    https://www.redhat.com/archives/linux-audit/2014-August/msg00065.html

v3: Rationalize and rename some function names and clean up get/put and free code.
    Rename several "watch" references to "mark".
    Rename audit_remove_rule() to audit_remove_mark_rule().
    Let audit_free_rule() take care of calling audit_remove_mark().
    Put audit_alloc_mark() arguments in same order as watch, tree and inode.
    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_remove_mark_rule().
    https://www.redhat.com/archives/linux-audit/2014-July/msg00000.html

v2: Misguided attempt to add in audit_exe similar to watches
    https://www.redhat.com/archives/linux-audit/2014-June/msg00066.html

v1.5: eparis' switch to fsnotify
    https://www.redhat.com/archives/linux-audit/2014-May/msg00046.html
    https://www.redhat.com/archives/linux-audit/2014-May/msg00066.html

v1: Change to path interface instead of inode
    https://www.redhat.com/archives/linux-audit/2014-May/msg00017.html

v0: Peter Moodie's original patches
    https://www.redhat.com/archives/linux-audit/2012-August/msg00033.html


Future step:
Get full-path notify working.


Eric Paris (1):
  audit: implement audit by executable

Richard Guy Briggs (3):
  audit: clean simple fsnotify implementation
  audit: convert audit_exe to audit_fsnotify
  audit: avoid double copying the audit_exe path string

 include/linux/audit.h      |    1 +
 include/uapi/linux/audit.h |    2 +
 kernel/Makefile            |    2 +-
 kernel/audit.h             |   33 ++++++
 kernel/audit_exe.c         |   50 +++++++++
 kernel/audit_fsnotify.c    |  246 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/audit_tree.c        |    2 +
 kernel/audit_watch.c       |    4 +
 kernel/auditfilter.c       |   63 +++++++++++-
 kernel/auditsc.c           |   16 +++
 10 files changed, 415 insertions(+), 4 deletions(-)
 create mode 100644 kernel/audit_exe.c
 create mode 100644 kernel/audit_fsnotify.c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]


#1183813 — [PATCH V6 1/4] audit: implement audit by executable

FromRichard Guy Briggs <rgb@redhat.com>
Date2015-07-14 18:00 +0200
Subject[PATCH V6 1/4] audit: implement audit by executable
Message-ID<pMaMG-4Md-19@gated-at.bofh.it>
In reply to#1183811
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.

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 <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             |   32 +++++++++++++
 kernel/audit_exe.c         |  109 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/auditfilter.c       |   44 ++++++++++++++++++
 kernel/auditsc.c           |   16 ++++++
 7 files changed, 205 insertions(+), 1 deletions(-)
 create mode 100644 kernel/audit_exe.c

diff --git a/include/linux/audit.h b/include/linux/audit.h
index c2e7e3a..95463a2 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 d3475e1..489aebc 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 60c302c..a7ea330 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -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
+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 d641f9b..3aca24f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -50,6 +50,7 @@ enum audit_state {
 
 /* Rule lists */
 struct audit_watch;
+struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
 
@@ -269,6 +270,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);
+
+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);
+
 #else
 #define audit_put_watch(w) {}
 #define audit_get_watch(w) {}
@@ -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)
+{
+	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..d4cc8b5
--- /dev/null
+++ b/kernel/audit_exe.c
@@ -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)
+{
+	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;
+
+	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 74cc077..09041b2 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -405,6 +405,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;
 }
@@ -539,6 +546,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;
 		}
 	}
 
@@ -615,6 +639,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;
 		case AUDIT_LOGINUID_SET:
 			if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
 				data->fields[i] = AUDIT_LOGINUID;
@@ -678,6 +707,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:
@@ -799,6 +835,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_dupe_exe(new, old);
+			break;
 		}
 		if (err) {
 			audit_free_rule(entry);
@@ -965,6 +1006,9 @@ 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 9fb9d1c..bf745c7 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>
@@ -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;
+				}
+			}
+		}
+			break;
 		case AUDIT_UID:
 			result = audit_uid_comparator(cred->uid, f->op, f->uid);
 			break;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186356 — Re: [PATCH V6 1/4] audit: implement audit by executable

FromPaul Moore <pmoore@redhat.com>
Date2015-07-17 03:20 +0200
SubjectRe: [PATCH V6 1/4] audit: implement audit by executable
Message-ID<pN2tH-7ap-5@gated-at.bofh.it>
In reply to#1183813
On Tuesday, July 14, 2015 11:50:23 AM Richard Guy Briggs wrote:
> 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.

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.

> 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 <rgb@redhat.com>
> Based-on-idea-by: Peter Moody <pmoody@google.com>

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.

> Cc: pmoody@google.com
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

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.
 
> diff --git a/kernel/audit.h b/kernel/audit.h
> index d641f9b..3aca24f 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -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) +{
> +	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 ...
 
> diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
> new file mode 100644
> index 0000000..d4cc8b5
> --- /dev/null
> +++ b/kernel/audit_exe.c
> @@ -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)
> +{
> +	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;

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.

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

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.

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

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9fb9d1c..bf745c7 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>
> @@ -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;
> +				}
> +			}
> +		}
> +			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
security @ redhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1183814 — [PATCH V6 4/4] audit: avoid double copying the audit_exe path string

FromRichard Guy Briggs <rgb@redhat.com>
Date2015-07-14 18:00 +0200
Subject[PATCH V6 4/4] audit: avoid double copying the audit_exe path string
Message-ID<pMaMG-4Md-21@gated-at.bofh.it>
In reply to#1183811
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 <rgb@redhat.com>
---
 kernel/audit_exe.c      |    8 ++++++--
 kernel/audit_fsnotify.c |    9 +--------
 kernel/auditfilter.c    |    2 +-
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
index 75ad4f2..09e4eb4 100644
--- a/kernel/audit_exe.c
+++ b/kernel/audit_exe.c
@@ -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;
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index a4e7b16..e57e08a 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -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;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f65c97f..f46ed69 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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;
 			}
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186375 — Re: [PATCH V6 4/4] audit: avoid double copying the audit_exe path string

FromPaul Moore <pmoore@redhat.com>
Date2015-07-17 04:00 +0200
SubjectRe: [PATCH V6 4/4] audit: avoid double copying the audit_exe path string
Message-ID<pN36q-7Ts-11@gated-at.bofh.it>
In reply to#1183814
On Tuesday, July 14, 2015 11:50:26 AM Richard Guy Briggs wrote:
> 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 <rgb@redhat.com>
> ---
>  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.

> diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
> index 75ad4f2..09e4eb4 100644
> --- a/kernel/audit_exe.c
> +++ b/kernel/audit_exe.c
> @@ -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;
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index a4e7b16..e57e08a 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -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;
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f65c97f..f46ed69 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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;
>  			}

-- 
paul moore
security @ redhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1183817 — [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify

FromRichard Guy Briggs <rgb@redhat.com>
Date2015-07-14 18:00 +0200
Subject[PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify
Message-ID<pMaMH-4Md-33@gated-at.bofh.it>
In reply to#1183811
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>

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 <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |    2 +-
 kernel/audit.h        |   33 +++---------------
 kernel/audit_exe.c    |   87 +++++++------------------------------------------
 kernel/audit_tree.c   |    2 +
 kernel/audit_watch.c  |    4 ++
 kernel/auditfilter.c  |   28 +++++++++++----
 6 files changed, 45 insertions(+), 111 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 95463a2..aee456f 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 491bd4b..eeaf054 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -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) {}
@@ -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
diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
index d4cc8b5..75ad4f2 100644
--- a/kernel/audit_exe.c
+++ b/kernel/audit_exe.c
@@ -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);
 }
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index b0f9877..94ecdab 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -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);
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 8f123d7..4aaa393 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -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);
 			list_del_rcu(&e->list);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bbb39ec..f65c97f 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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);
 	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:
 		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 */
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186370 — Re: [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify

FromPaul Moore <pmoore@redhat.com>
Date2015-07-17 04:00 +0200
SubjectRe: [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify
Message-ID<pN36p-7Ts-1@gated-at.bofh.it>
In reply to#1183817
On Tuesday, July 14, 2015 11:50:25 AM Richard Guy Briggs wrote:
> 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>
> 
> 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 <eparis@redhat.com>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

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.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 95463a2..aee456f 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 491bd4b..eeaf054 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -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) {}
> @@ -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
> diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
> index d4cc8b5..75ad4f2 100644
> --- a/kernel/audit_exe.c
> +++ b/kernel/audit_exe.c
> @@ -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);
>  }
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index b0f9877..94ecdab 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -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);
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 8f123d7..4aaa393 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -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);
>  			list_del_rcu(&e->list);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index bbb39ec..f65c97f 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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);
>  	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:
>  		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 */

-- 
paul moore
security @ redhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1183818 — [PATCH V6 2/4] audit: clean simple fsnotify implementation

FromRichard Guy Briggs <rgb@redhat.com>
Date2015-07-14 18:00 +0200
Subject[PATCH V6 2/4] audit: clean simple fsnotify implementation
Message-ID<pMaMH-4Md-37@gated-at.bofh.it>
In reply to#1183811
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 <eparis@redhat.com>

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 <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/Makefile         |    2 +-
 kernel/audit.h          |   22 ++++
 kernel/audit_fsnotify.c |  253 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/auditfilter.c    |    5 +-
 4 files changed, 279 insertions(+), 3 deletions(-)
 create mode 100644 kernel/audit_fsnotify.c

diff --git a/kernel/Makefile b/kernel/Makefile
index a7ea330..397109e 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -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
diff --git a/kernel/audit.h b/kernel/audit.h
index 3aca24f..491bd4b 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -50,6 +50,7 @@ enum audit_state {
 
 /* Rule lists */
 struct audit_watch;
+struct audit_fsnotify_mark;
 struct audit_exe;
 struct audit_tree;
 struct audit_chunk;
@@ -253,6 +254,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[];
 
@@ -271,6 +273,12 @@ 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);
 
+extern struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len);
+extern char *audit_mark_path(struct audit_fsnotify_mark *mark);
+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);
@@ -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
 
+#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)
+{
+	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..a4e7b16
--- /dev/null
+++ b/kernel/audit_fsnotify.c
@@ -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.
+ */
+
+#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_mark_free(struct audit_fsnotify_mark *audit_mark)
+{
+	kfree(audit_mark->path);
+	kfree(audit_mark);
+}
+
+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);
+}
+
+#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(struct audit_krule *krule, char *pathname, int len)
+{
+	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);
+		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) {
+		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)
+{
+	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);
+}
+
+void audit_remove_mark_rule(struct audit_krule *krule)
+{
+	struct audit_fsnotify_mark *mark = krule->exe;
+
+	audit_remove_mark(mark);
+}
+
+static void audit_autoremove_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);
+
+	audit_mark_log_rule_change(audit_mark, "autoremove_rule");
+	audit_del_rule(entry);
+}
+
+/* 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;
+
+	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_autoremove_mark_rule(audit_mark);
+
+	return 0;
+}
+
+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_mark_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 09041b2..bbb39ec 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_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--;
 
-	if (!audit_match_signal(entry))
+	if (!match)
 		audit_signals--;
 #endif
 	mutex_unlock(&audit_filter_mutex);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186366 — Re: [PATCH V6 2/4] audit: clean simple fsnotify implementation

FromPaul Moore <pmoore@redhat.com>
Date2015-07-17 03:50 +0200
SubjectRe: [PATCH V6 2/4] audit: clean simple fsnotify implementation
Message-ID<pN2WJ-7I8-1@gated-at.bofh.it>
In reply to#1183818
On Tuesday, July 14, 2015 11:50:24 AM Richard Guy Briggs wrote:
> 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 <eparis@redhat.com>
> 
> 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().

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

> Based-on-code-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

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

> diff --git a/kernel/Makefile b/kernel/Makefile
> index a7ea330..397109e 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -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
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 3aca24f..491bd4b 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -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
> 
> +#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) +{
> +	BUG();
> +	return 0;
> +}

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

> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> new file mode 100644
> index 0000000..a4e7b16
> --- /dev/null
> +++ b/kernel/audit_fsnotify.c
> @@ -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.
> + */

...

> +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?

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

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.

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

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).

> +struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule,
> char *pathname, int len) +{
> +	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;
> +	}

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

> +	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?

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

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

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

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

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

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 09041b2..bbb39ec 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_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--;
> 
> -	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
security @ redhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1184760

FromSteve Grubb <sgrubb@redhat.com>
Date2015-07-15 14:30 +0200
Message-ID<pMtZ0-7p7-17@gated-at.bofh.it>
In reply to#1183811
On Tuesday, July 14, 2015 11:50:22 AM Richard Guy Briggs wrote:
> Please see the accompanying userspace patchset:
> 	https://www.redhat.com/archives/linux-audit/2015-July/thread.html
> 	[[PATCH V2] 0/2] Log on the future execution of a path
> The userspace interface is not expected to change appreciably unless
> something important has been overlooked.  Setting and deleting rules works
> as expected.
> 
> If the path does not exist at rule creation time, it will be re-evaluated
> every time there is a change to the parent directory at which point the
> change in device and inode will be noted.

Thanks for doing this. Its a much needed feature.

In looking over it...does this add an AUDIT_VERSION_ define and use it in the 
feature mask so that I can tell what kernels support this? I might have missed 
it, but I can't find one.

Thanks,
-Steve


> Here's a sample run:
> Test for addition, trigger and deletion of tree executable rule:
> # auditctl -a always,exit -S all -F dir=/tmp -F exe=/usr/bin/touch -F
> key=exetest_tree ----
> time->Sat Jul 11 10:41:50 2015
> type=CONFIG_CHANGE msg=audit(1436629310.720:44711): auid=0 ses=1
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="add_rule"
> key="exetest_tree" list=4 res=1 ----
> 
> # /usr/bin/touch /tmp/test
> ----
> time->Sat Jul 11 10:41:50 2015
> type=PROCTITLE msg=audit(1436629310.757:44712):
> proctitle=2F7573722F62696E2F746F756368002F746D702F74657374 type=PATH
> msg=audit(1436629310.757:44712): item=1 name="/tmp/test" inode=166932
> dev=00:24 mode=0100644 ouid=0 ogid=0 rdev=00:00
> obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE type=PATH
> msg=audit(1436629310.757:44712): item=0 name="/tmp/" inode=11525 dev=00:24
> mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0
> nametype=PARENT type=CWD msg=audit(1436629310.757:44712):  cwd="/root"
> type=SYSCALL msg=audit(1436629310.757:44712): arch=c000003e syscall=2
> success=yes exit=3 a0=7ffdee2f9e27 a1=941 a2=1b6 a3=691 items=2 ppid=17655
> pid=17762 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> tty=ttyS0 ses=1 comm="touch" exe="/usr/bin/touch"
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key="exetest_tree" ----
> 
> # auditctl -d always,exit -S all -F dir=/tmp -F exe=/usr/bin/touch -F
> key=exetest_tree ----
> time->Sat Jul 11 10:41:50 2015
> type=CONFIG_CHANGE msg=audit(1436629310.839:44713): auid=0 ses=1
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="remove_rule"
> key="exetest_tree" list=4 res=1 ----
> 
> 
> Revision history:
> v6: Explicitly declare prototypes as external.
>     Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch,
> lsm_field. Rebased on v4.1.
>     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.
>     Add audit_remove_mark_rule() to provide similar interface as
>       audit_remove_{watch,tree}_rule().
>     Simplify stubs to defines.
>     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().
>     Return -ENOMEM rather than null in case of memory allocation failure for
> audit_mark in audit_alloc_mark().
>     Rename audit_free_mark() to audit_mark_free() to avoid association with
>       {i,d,fa}notify_free_mark() and audit_watch_free_mark().
>     Clean up exe with similar interface as watch and tree.
>     Clean up audit exe mark just before audit_free_rule() rather than in it
> to avoid mutex in software interrupt context.
>     Fixed bug in audit_dupe_exe() that returned error rather than valid
> pointer.
> 
> v5: Revert patch "Let audit_free_rule() take care of calling
>     audit_remove_mark()." since it caused a group mark deadlock.
>     https://www.redhat.com/archives/linux-audit/2014-October/msg00024.html
> 
> v4: Re-order and squash down fixups
>     Fix audit_dup_exe() to copy pathname string before calling
> audit_alloc_mark().
> https://www.redhat.com/archives/linux-audit/2014-August/msg00065.html
> 
> v3: Rationalize and rename some function names and clean up get/put and free
> code. Rename several "watch" references to "mark".
>     Rename audit_remove_rule() to audit_remove_mark_rule().
>     Let audit_free_rule() take care of calling audit_remove_mark().
>     Put audit_alloc_mark() arguments in same order as watch, tree and inode.
> 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_remove_mark_rule().
>     https://www.redhat.com/archives/linux-audit/2014-July/msg00000.html
> 
> v2: Misguided attempt to add in audit_exe similar to watches
>     https://www.redhat.com/archives/linux-audit/2014-June/msg00066.html
> 
> v1.5: eparis' switch to fsnotify
>     https://www.redhat.com/archives/linux-audit/2014-May/msg00046.html
>     https://www.redhat.com/archives/linux-audit/2014-May/msg00066.html
> 
> v1: Change to path interface instead of inode
>     https://www.redhat.com/archives/linux-audit/2014-May/msg00017.html
> 
> v0: Peter Moodie's original patches
>     https://www.redhat.com/archives/linux-audit/2012-August/msg00033.html
> 
> 
> Future step:
> Get full-path notify working.
> 
> 
> Eric Paris (1):
>   audit: implement audit by executable
> 
> Richard Guy Briggs (3):
>   audit: clean simple fsnotify implementation
>   audit: convert audit_exe to audit_fsnotify
>   audit: avoid double copying the audit_exe path string
> 
>  include/linux/audit.h      |    1 +
>  include/uapi/linux/audit.h |    2 +
>  kernel/Makefile            |    2 +-
>  kernel/audit.h             |   33 ++++++
>  kernel/audit_exe.c         |   50 +++++++++
>  kernel/audit_fsnotify.c    |  246
> ++++++++++++++++++++++++++++++++++++++++++++ kernel/audit_tree.c        |  
>  2 +
>  kernel/audit_watch.c       |    4 +
>  kernel/auditfilter.c       |   63 +++++++++++-
>  kernel/auditsc.c           |   16 +++
>  10 files changed, 415 insertions(+), 4 deletions(-)
>  create mode 100644 kernel/audit_exe.c
>  create mode 100644 kernel/audit_fsnotify.c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web