10:06 p.m.
This is a part of Peter Moody, my and Eric Paris' work to implement
audit by executable name.
Please see the accompanying userspace patch:
https://www.redhat.com/archives/linux-audit/2014-May/msg00019.html
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:
# /usr/local/sbin/auditctl -a always,exit -F dir=/tmp -F exe=/bin/touch -F key=touch_tmp
# /usr/local/sbin/ausearch --start recent -k touch_tmp
time->Mon Jun 30 14:15:06 2014
type=CONFIG_CHANGE msg=audit(1404152106.683:149): auid=0 ses=1 subj=unconfined_u
:unconfined_r:auditctl_t:s0-s0:c0.c1023 op="add_rule" key="touch_tmp"
list=4 res =1
# /usr/local/sbin/auditctl -l
-a always,exit -S all -F dir=/tmp -F exe=/bin/touch -F key=touch_tmp
# touch /tmp/test
# /usr/local/sbin/ausearch --start recent -k touch_tmp
time->Wed Jul 2 12:18:47 2014
type=UNKNOWN[1327] msg=audit(1404317927.319:132):
proctitle=746F756368002F746D702F74657374
type=PATH msg=audit(1404317927.319:132): item=1 name="/tmp/test" inode=25997
dev=00:20 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0
nametype=CREATE
type=PATH msg=audit(1404317927.319:132): item=0 name="/tmp/" inode=11144
dev=00:20 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0
nametype=PARENT
type=CWD msg=audit(1404317927.319:132): cwd="/root"
type=SYSCALL msg=audit(1404317927.319:132): arch=c000003e syscall=2 success=yes exit=3
a0=7ffffa403dd5 a1=941 a2=1b6 a3=34b65b2c6c items=2 ppid=4321 pid=6436 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="touch_tmp"
Revision history:
v5: Revert patch "Let audit_free_rule() take care of calling
audit_remove_mark()." since it caused a group mark deadlock.
v4: Re-order and squash down fixups
Fix audit_dup_exe() to copy pathname string before calling audit_alloc_mark().
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
Next step:
Get full-path notify working.
Eric Paris (3):
audit: implement audit by executable
audit: clean simple fsnotify implementation
audit: convert audit_exe to audit_fsnotify
Richard Guy Briggs (2):
audit: avoid double copying the audit_exe path string
Revert "fixup! audit: clean simple fsnotify implementation"
include/linux/audit.h | 1 +
include/uapi/linux/audit.h | 2 +
kernel/Makefile | 2 +-
kernel/audit.h | 39 +++++++
kernel/audit_exe.c | 49 +++++++++
kernel/audit_fsnotify.c | 237 ++++++++++++++++++++++++++++++++++++++++++++
kernel/auditfilter.c | 52 +++++++++-
kernel/auditsc.c | 16 +++
8 files changed, 395 insertions(+), 3 deletions(-)
create mode 100644 kernel/audit_exe.c
create mode 100644 kernel/audit_fsnotify.c
10:06 p.m.
New subject: [PATCH V5 1/5] audit: implement audit by executable
From: Eric Paris <eparis(a)redhat.com>
This patch implements the ability to filter on the executable. It is
clearly incomplete! This patch adds the inode/dev of the executable at
the moment the rule is loaded. It does not update if the executable is
updated/moved/whatever. That should be added. But at this moment, this
patch works.
Based-on-user-interface-by: Richard Guy Briggs <rgb(a)redhat.com>
Cc: rgb(a)redhat.com
Based-on-idea-by: Peter Moody <pmoody(a)google.com>
Cc: pmoody(a)google.com
Signed-off-by: Eric Paris <eparis(a)redhat.com>
Signed-off-by: Richard Guy Briggs <rgb(a)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 36dffec..ce51204 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 4d100c8..101d344 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 f2a8b62..60def04 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -63,7 +63,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 3cdffad..7825c7e 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -56,6 +56,7 @@ enum audit_state {
/* Rule lists */
struct audit_watch;
+struct audit_exe;
struct audit_tree;
struct audit_chunk;
@@ -279,6 +280,13 @@ extern int audit_add_watch(struct audit_krule *krule, struct
list_head **list);
extern void audit_remove_watch_rule(struct audit_krule *krule);
extern char *audit_watch_path(struct audit_watch *watch);
extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t dev);
+
+int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op);
+void audit_remove_exe_rule(struct audit_krule *krule);
+char *audit_exe_path(struct audit_exe *exe);
+int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
+int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe);
+
#else
#define audit_put_watch(w) {}
#define audit_get_watch(w) {}
@@ -288,6 +296,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_dup_exe(struct audit_krule *new, struct audit_krule *old)
+{
+ BUG();
+ return -EINVAL
+}
+static inline int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
+{
+ BUG();
+ return 0;
+}
#endif /* CONFIG_AUDIT_WATCH */
#ifdef CONFIG_AUDIT_TREE
diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
new file mode 100644
index 0000000..ec3231b
--- /dev/null
+++ b/kernel/audit_exe.c
@@ -0,0 +1,109 @@
+/* audit_exe.c -- filtering of audit events
+ *
+ * Copyright 2014 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/audit.h>
+#include <linux/mutex.h>
+#include <linux/fs.h>
+#include <linux/namei.h>
+#include <linux/slab.h>
+#include "audit.h"
+
+struct audit_exe {
+ char *pathname;
+ unsigned long ino;
+ dev_t dev;
+};
+
+/* Translate a watch string to kernel respresentation. */
+int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op)
+{
+ struct audit_exe *exe;
+ struct path path;
+ struct dentry *dentry;
+ unsigned long ino;
+ dev_t dev;
+
+ if (pathname[0] != '/' || pathname[len-1] == '/')
+ return -EINVAL;
+
+ dentry = kern_path_locked(pathname, &path);
+ if (IS_ERR(dentry))
+ return PTR_ERR(dentry);
+ mutex_unlock(&path.dentry->d_inode->i_mutex);
+
+ if (!dentry->d_inode)
+ return -ENOENT;
+ dev = dentry->d_inode->i_sb->s_dev;
+ ino = dentry->d_inode->i_ino;
+ dput(dentry);
+
+ exe = kmalloc(sizeof(*exe), GFP_KERNEL);
+ if (!exe)
+ return -ENOMEM;
+ exe->ino = ino;
+ exe->dev = dev;
+ exe->pathname = pathname;
+ krule->exe = exe;
+
+ return 0;
+}
+
+void audit_remove_exe_rule(struct audit_krule *krule)
+{
+ struct audit_exe *exe;
+
+ exe = krule->exe;
+ krule->exe = NULL;
+ kfree(exe->pathname);
+ kfree(exe);
+}
+
+char *audit_exe_path(struct audit_exe *exe)
+{
+ return exe->pathname;
+}
+
+int audit_dup_exe(struct audit_krule *new, struct audit_krule *old)
+{
+ struct audit_exe *exe;
+
+ exe = kmalloc(sizeof(*exe), GFP_KERNEL);
+ if (!exe)
+ return -ENOMEM;
+
+ exe->pathname = kstrdup(old->exe->pathname, GFP_KERNEL);
+ if (!exe->pathname) {
+ kfree(exe);
+ return -ENOMEM;
+ }
+
+ exe->ino = old->exe->ino;
+ exe->dev = old->exe->dev;
+ new->exe = exe;
+
+ return 0;
+}
+
+int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
+{
+ if (tsk->mm->exe_file->f_inode->i_ino != exe->ino)
+ return 0;
+ if (tsk->mm->exe_file->f_inode->i_sb->s_dev != exe->dev)
+ return 0;
+ return 1;
+}
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 5675916..d9da99e 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;
}
@@ -553,6 +560,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;
}
}
@@ -629,6 +653,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule
*krule)
data->buflen += data->values[i] =
audit_pack_string(&bufp, krule->filterkey);
break;
+ case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
+ data->buflen += data->values[i] =
+ audit_pack_string(&bufp, audit_exe_path(krule->exe));
+ break;
default:
data->values[i] = f->val;
}
@@ -684,6 +713,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:
@@ -805,6 +841,11 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
err = -ENOMEM;
else
new->filterkey = fk;
+ break;
+ case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
+ err = audit_dup_exe(new, old);
+ break;
}
if (err) {
audit_free_rule(entry);
@@ -973,6 +1014,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 8933572..9460336 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 "audit.h"
@@ -464,6 +466,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
10:06 p.m.
New subject: [PATCH V5 2/5] audit: clean simple fsnotify implementation
From: Eric Paris <eparis(a)redhat.com>
This is to be used to audit by executable rules, but audit watches
should be able to share this code eventually.
At the moment the audit watch code is a lot more complex, that code only
creates one fsnotify watch per parent directory. That 'audit_parent' in
turn has a list of 'audit_watches' which contain the name, ino, dev of
the specific object we care about. This just creates one fsnotify watch
per object we care about. So if you watch 100 inodes in /etc this code
will create 100 fsnotify watches on /etc. The audit_watch code will
instead create 1 fsnotify watch on /etc (the audit_parent) and then 100
individual watches chained from that fsnotify mark.
We should be able to convert the audit_watch code to do one fsnotify
mark per watch and simplify things/remove a whole lot of code. After
that conversion we should be able to convert the audit_fsnotify code to
support that hierarchy if the optomization is necessary.
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_remove_mark_rule().
RGB: Rename several "watch" references to "mark".
RGB: Rename audit_remove_rule() to audit_remove_mark_rule().
RGB: Let audit_free_rule() take care of calling audit_remove_mark().
RGB: Put audit_alloc_mark() arguments in same order as watch, tree and inode.
RGB: Remove space from audit log value text in audit_remove_mark_rule().
Signed-off-by: Eric Paris <eparis(a)redhat.com>
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/Makefile | 2 +-
kernel/audit.h | 29 ++++++
kernel/audit_fsnotify.c | 245 +++++++++++++++++++++++++++++++++++++++++++++++
kernel/auditfilter.c | 10 +-
4 files changed, 280 insertions(+), 6 deletions(-)
create mode 100644 kernel/audit_fsnotify.c
diff --git a/kernel/Makefile b/kernel/Makefile
index 60def04..e82583f 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -63,7 +63,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 7825c7e..b8ecc06 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -56,6 +56,7 @@ enum audit_state {
/* Rule lists */
struct audit_watch;
+struct audit_fsnotify_mark;
struct audit_exe;
struct audit_tree;
struct audit_chunk;
@@ -266,6 +267,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[];
@@ -281,6 +283,11 @@ extern void audit_remove_watch_rule(struct audit_krule *krule);
extern char *audit_watch_path(struct audit_watch *watch);
extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t dev);
+struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname,
int len);
+char *audit_mark_path(struct audit_fsnotify_mark *mark);
+void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
+int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
+
int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op);
void audit_remove_exe_rule(struct audit_krule *krule);
char *audit_exe_path(struct audit_exe *exe);
@@ -296,6 +303,28 @@ int audit_exe_compare(struct task_struct *tsk, struct audit_exe
*exe);
#define audit_watch_path(w) ""
#define audit_watch_compare(w, i, d) 0
+static inline struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule,
char *pathname, int len)
+{
+ return ERR_PTR(-EINVAL);
+}
+
+static inline char *audit_mark_path(struct audit_fsnotify_mark *mark)
+{
+ BUG();
+ return "";
+}
+
+static inline void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
+{
+ BUG();
+}
+
+static inline int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino,
dev_t dev)
+{
+ BUG();
+ return 0;
+}
+
static inline int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len,
u32 op)
{
return -EINVAL;
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
new file mode 100644
index 0000000..f4b3e66
--- /dev/null
+++ b/kernel/audit_fsnotify.c
@@ -0,0 +1,245 @@
+/* audit_fsnotify.c -- tracking inodes
+ *
+ * Copyright 2003-2009 Red Hat, Inc.
+ * Copyright 2005 Hewlett-Packard Development Company, L.P.
+ * Copyright 2005 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/audit.h>
+#include <linux/kthread.h>
+#include <linux/mutex.h>
+#include <linux/fs.h>
+#include <linux/fsnotify_backend.h>
+#include <linux/namei.h>
+#include <linux/netlink.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/security.h>
+#include "audit.h"
+
+/*
+ * this mark lives on the parent directory of the inode in question.
+ * but dev, ino, and path are about the child
+ */
+struct audit_fsnotify_mark {
+ dev_t dev; /* associated superblock device */
+ unsigned long ino; /* associated inode number */
+ char *path; /* insertion path */
+ struct fsnotify_mark mark; /* fsnotify mark on the inode */
+ struct audit_krule *rule;
+};
+
+/* fsnotify handle. */
+static struct fsnotify_group *audit_fsnotify_group;
+
+/* fsnotify events we care about. */
+#define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
+ FS_MOVE_SELF | FS_EVENT_ON_CHILD)
+
+static void audit_free_mark(struct audit_fsnotify_mark *audit_mark)
+{
+ kfree(audit_mark->path);
+ kfree(audit_mark);
+}
+
+static void audit_free_fsnotify_mark(struct fsnotify_mark *mark)
+{
+ struct audit_fsnotify_mark *audit_mark;
+
+ audit_mark = container_of(mark, struct audit_fsnotify_mark, mark);
+ audit_free_mark(audit_mark);
+}
+
+#if 0 /* not sure if we need these... */
+static void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
+{
+ if (likely(audit_mark))
+ fsnotify_get_mark(&audit_mark->mark);
+}
+
+static void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
+{
+ if (likely(audit_mark))
+ fsnotify_put_mark(&audit_mark->mark);
+}
+#endif
+
+char *audit_mark_path(struct audit_fsnotify_mark *mark)
+{
+ return mark->path;
+}
+
+int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev)
+{
+ if (mark->ino == (unsigned long)-1)
+ return 0;
+ return (mark->ino == ino) && (mark->dev == dev);
+}
+
+struct audit_fsnotify_mark *audit_alloc_mark(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);
+ goto out;
+ }
+
+ fsnotify_init_mark(&audit_mark->mark, audit_free_fsnotify_mark);
+ audit_mark->mark.mask = AUDIT_FS_EVENTS;
+ audit_mark->path = local_pathname;
+ audit_mark->ino = ino;
+ audit_mark->dev = dev;
+ audit_mark->rule = krule;
+
+ ret = fsnotify_add_mark(&audit_mark->mark, audit_fsnotify_group, inode, NULL,
true);
+ if (ret < 0) {
+ audit_free_mark(audit_mark);
+ audit_mark = ERR_PTR(ret);
+ }
+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);
+}
+
+static void audit_remove_mark_rule(struct audit_fsnotify_mark *audit_mark)
+{
+ struct audit_krule *rule = audit_mark->rule;
+ struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
+
+ audit_mark_log_rule_change(audit_mark, "remove_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_remove_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 d9da99e..9eb29c0 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -97,6 +97,8 @@ static inline void audit_free_rule(struct audit_entry *e)
/* some rules don't have associated watches */
if (erule->watch)
audit_put_watch(erule->watch);
+ if (erule->exe)
+ audit_remove_mark(erule->exe);
if (erule->fields)
for (i = 0; i < erule->field_count; i++)
audit_free_lsm_field(&erule->fields[i]);
@@ -985,7 +987,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;
@@ -993,6 +995,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 ||
@@ -1014,9 +1017,6 @@ 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);
@@ -1025,7 +1025,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
10:06 p.m.
New subject: [PATCH V5 3/5] audit: convert audit_exe to audit_fsnotify
From: Eric Paris <eparis(a)redhat.com>
Instead of just hard coding the ino and dev of the executable we care
about at the moment the rule is inserted into the kernel, use the new
audit_fsnotify infrastructure. This means that if the inode in question
is unlinked and creat'd (aka updated) the rule will just continue to
work.
Signed-off-by: Eric Paris <eparis(a)redhat.com>
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 2 +-
kernel/audit.h | 32 +++---------------
kernel/audit_exe.c | 87 +++++++------------------------------------------
kernel/auditfilter.c | 15 +++++---
4 files changed, 27 insertions(+), 109 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index ce51204..0ffa268 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 b8ecc06..9821732 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -57,7 +57,6 @@ enum audit_state {
/* Rule lists */
struct audit_watch;
struct audit_fsnotify_mark;
-struct audit_exe;
struct audit_tree;
struct audit_chunk;
@@ -288,11 +287,8 @@ char *audit_mark_path(struct audit_fsnotify_mark *mark);
void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev);
-int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op);
-void audit_remove_exe_rule(struct audit_krule *krule);
-char *audit_exe_path(struct audit_exe *exe);
int audit_dup_exe(struct audit_krule *new, struct audit_krule *old);
-int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe);
+int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark);
#else
#define audit_put_watch(w) {}
@@ -319,36 +315,18 @@ static inline void audit_remove_mark(struct audit_fsnotify_mark
*audit_mark)
BUG();
}
-static inline int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino,
dev_t dev)
+static inline int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
*mark)
{
BUG();
- return 0;
-}
-
-static inline int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len,
u32 op)
-{
return -EINVAL;
}
-static inline void audit_remove_exe_rule(struct audit_krule *krule)
-{
- BUG();
- return 0;
-}
-static inline char *audit_exe_path(struct audit_exe *exe)
-{
- BUG();
- return "";
-}
+
static inline int audit_dup_exe(struct audit_krule *new, struct audit_krule *old)
{
BUG();
- return -EINVAL
-}
-static inline int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
-{
- 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 ec3231b..0c7ee8d 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_dup_exe(struct audit_krule *new, struct audit_krule *old)
{
- struct audit_exe *exe;
-
- exe = kmalloc(sizeof(*exe), GFP_KERNEL);
- if (!exe)
- return -ENOMEM;
+ struct audit_fsnotify_mark *audit_mark;
+ char *pathname;
- exe->pathname = kstrdup(old->exe->pathname, GFP_KERNEL);
- if (!exe->pathname) {
- kfree(exe);
- return -ENOMEM;
- }
+ pathname = audit_mark_path(old->exe);
- exe->ino = old->exe->ino;
- exe->dev = old->exe->dev;
- new->exe = exe;
+ audit_mark = audit_alloc_mark(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/auditfilter.c b/kernel/auditfilter.c
index 9eb29c0..fff92cf 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -428,6 +428,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))
@@ -573,11 +574,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;
}
}
@@ -658,7 +661,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule
*krule)
case AUDIT_EXE:
case AUDIT_EXE_CHILDREN:
data->buflen += data->values[i] =
- audit_pack_string(&bufp, audit_exe_path(krule->exe));
+ audit_pack_string(&bufp, audit_mark_path(krule->exe));
break;
default:
data->values[i] = f->val;
@@ -718,8 +721,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:
--
1.7.1
10:06 p.m.
New subject: [PATCH V5 4/5] audit: avoid double copying the audit_exe path string
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(a)redhat.com>
---
kernel/audit_exe.c | 5 ++++-
kernel/audit_fsnotify.c | 12 ++----------
kernel/auditfilter.c | 2 +-
3 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
index 0c7ee8d..ff6e3d6 100644
--- a/kernel/audit_exe.c
+++ b/kernel/audit_exe.c
@@ -27,10 +27,13 @@ int audit_dup_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))
+ kfree(pathname);
return PTR_ERR(audit_mark);
new->exe = audit_mark;
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index f4b3e66..6daf5c3 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,20 +114,13 @@ 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);
+ if (unlikely(!audit_mark))
goto out;
- }
fsnotify_init_mark(&audit_mark->mark, audit_free_fsnotify_mark);
audit_mark->mark.mask = AUDIT_FS_EVENTS;
- audit_mark->path = local_pathname;
+ audit_mark->path = pathname;
audit_mark->ino = ino;
audit_mark->dev = dev;
audit_mark->rule = krule;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index fff92cf..570e79a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -575,8 +575,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
10:06 p.m.
New subject: [PATCH V5 5/5] Revert "fixup! audit: clean simple fsnotify implementation"
This reverts commit 826a3dbd65f0fdb1d7ddfa2849de700f360e1494.
"Let audit_free_rule() take care of calling audit_remove_mark()."
It was causing a group mark deadlock.
With kernel locking debugging config options enabled, I get the following
output. Could I get some help interpreting it?
I thought I had done a fairly careful job of justifying to myself that the mark
remove should be moved from audit_free_rule() to audit_del_rule(), but
evidently it wasn't happy.
[root@c1-f18 ~]# killall auditd;sleep 1;/usr/local/sbin/auditd
[root@c1-f18 ~]# /usr/local/sbin/auditctl -l
No rules
[root@c1-f18 ~]# /usr/local/sbin/auditctl -a always,exit -F dir=/tmp -F
exe=/usr/sbin/touch -F key=touch_tmp
[root@c1-f18 ~]# /usr/local/sbin/auditctl -l
-a always,exit -S all -F dir=/tmp -F exe=/usr/sbin/touch -F key=touch_tmp
[root@c1-f18 ~]# /usr/local/sbin/auditctl -d always,exit -F dir=/tmp -F
exe=/usr/sbin/touch -F key=touch_tmp
[root@c1-f18 ~]# [ 53.824114] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:616
[ 53.825152] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/3
[ 53.826154]
[ 53.826349] =================================
[ 53.826854] [ INFO: inconsistent lock state ]
[ 53.827108] 3.14.0-bz837856-audit-filter-name-v2+ #280 Not tainted
[ 53.827108] ---------------------------------
[ 53.827108] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
[ 53.827108] swapper/3/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
[ 53.827108] (&group->mark_mutex/1){+.?...}, at: [<ffffffff8128ebf3>]
fsnotify_destroy_mark+0x33/0x60
[ 53.827108] {SOFTIRQ-ON-W} state was registered at:
[ 53.827108] [<ffffffff810ef0c3>] __lock_acquire+0x7d3/0x1890
[ 53.827108] [<ffffffff810f022a>] lock_acquire+0xaa/0x180
[ 53.827108] [<ffffffff817ec3dd>] mutex_lock_nested+0x6d/0x4e0
[ 53.827108] [<ffffffff8128eb1b>] fsnotify_clear_marks_by_group_flags+0x3b/0xc0
[ 53.827108] [<ffffffff8128ebb3>] fsnotify_clear_marks_by_group+0x13/0x20
[ 53.827108] [<ffffffff8128db66>] fsnotify_destroy_group+0x16/0x50
[ 53.827108] [<ffffffff812901b8>] inotify_release+0x68/0x80
[ 53.827108] [<ffffffff81240705>] __fput+0x115/0x2a0
[ 53.827108] [<ffffffff8124095e>] ____fput+0xe/0x10
[ 53.827108] [<ffffffff810b391d>] task_work_run+0xad/0xe0
[ 53.827108] [<ffffffff81018df7>] do_notify_resume+0x97/0xd0
[ 53.827108] [<ffffffff817fb622>] int_signal+0x12/0x17
[ 53.827108] irq event stamp: 2397788
[ 53.827108] hardirqs last enabled at (2397788): [<ffffffff81101739>]
vprintk_emit+0x119/0x630
[ 53.827108] hardirqs last disabled at (2397787): [<ffffffff811016c4>]
vprintk_emit+0xa4/0x630
[ 53.827108] softirqs last enabled at (2397606): [<ffffffff8108e40c>]
_local_bh_enable+0x9c/0xd0
[ 53.827108] softirqs last disabled at (2397607): [<ffffffff8108f465>]
irq_exit+0x105/0x110
[ 53.827108]
[ 53.827108] other info that might help us debug this:
[ 53.827108] Possible unsafe locking scenario:
[ 53.827108]
[ 53.827108] CPU0
[ 53.827108] ----
[ 53.827108] lock(&group->mark_mutex/1);
[ 53.827108] <Interrupt>
[ 53.827108] lock(&group->mark_mutex/1);
[ 53.827108]
[ 53.827108] *** DEADLOCK ***
[ 53.827108]
[ 53.827108] 1 lock held by swapper/3/0:
[ 53.827108] #0: (rcu_callback){.+....}, at: [<ffffffff81114247>]
rcu_process_callbacks+0x577/0xd00
[ 53.827108]
[ 53.827108] stack backtrace:
[ 53.827108] CPU: 3 PID: 0 Comm: swapper/3 Not tainted
3.14.0-bz837856-audit-filter-name-v2+ #280
[ 53.827108] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2007
[ 53.827108] ffffffff825e90a0 ffff88003e403b18 ffffffff817e8908 ffff88003d2e0000
[ 53.827108] ffff88003d2e0000 ffff88003e403b68 ffffffff810ecf4f 0000000000000001
[ 53.827108] ffffffff00000001 0000000000000000 ffffffff825e9138 ffff88003d2e0848
[ 53.827108] Call Trace:
[ 53.827108] <IRQ> [<ffffffff817e8908>] dump_stack+0x51/0x71
[ 53.827108] [<ffffffff810ecf4f>] print_usage_bug+0x22f/0x280
[ 53.827108] [<ffffffff810edec2>] mark_lock+0x392/0x470
[ 53.827108] [<ffffffff810ef078>] __lock_acquire+0x788/0x1890
[ 53.827108] [<ffffffff8101b696>] ? show_stack_log_lvl+0xb6/0x1a0
[ 53.827108] [<ffffffff810f022a>] lock_acquire+0xaa/0x180
[ 53.827108] [<ffffffff8128ebf3>] ? fsnotify_destroy_mark+0x33/0x60
[ 53.827108] [<ffffffff8128ebf3>] ? fsnotify_destroy_mark+0x33/0x60
[ 53.827108] [<ffffffff817ec3dd>] mutex_lock_nested+0x6d/0x4e0
[ 53.827108] [<ffffffff8128ebf3>] ? fsnotify_destroy_mark+0x33/0x60
[ 53.827108] [<ffffffff810cfe93>] ? sched_clock_local+0x43/0xb0
[ 53.827108] [<ffffffff810d0028>] ? sched_clock_cpu+0x128/0x130
[ 53.827108] [<ffffffff8128ebf3>] fsnotify_destroy_mark+0x33/0x60
[ 53.827108] [<ffffffff811579f1>] audit_remove_mark+0x21/0x30
[ 53.827108] [<ffffffff8114fff8>] audit_free_rule_rcu+0x38/0xc0
[ 53.827108] [<ffffffff81114924>] rcu_process_callbacks+0xc54/0xd00
[ 53.827108] [<ffffffff81114247>] ? rcu_process_callbacks+0x577/0xd00
[ 53.827108] [<ffffffff817f0460>] ? _raw_spin_unlock_irq+0x30/0x50
[ 53.827108] [<ffffffff81097fe0>] ? run_timer_softirq+0x1c0/0x350
[ 53.827108] [<ffffffff8114ffc0>] ? audit_filter_type+0x260/0x260
[ 53.827108] [<ffffffff8108eee4>] __do_softirq+0x134/0x530
[ 53.827108] [<ffffffff8108f465>] irq_exit+0x105/0x110
[ 53.827108] [<ffffffff817fd65a>] smp_apic_timer_interrupt+0x4a/0x60
[ 53.827108] [<ffffffff817fbf72>] apic_timer_interrupt+0x72/0x80
[ 53.827108] <EOI> [<ffffffff81110f34>] ?
rcu_eqs_enter_common+0x1c4/0x410
[ 53.827108] [<ffffffff8105cd96>] ? native_safe_halt+0x6/0x10
[ 53.827108] [<ffffffff810ee5bd>] ? trace_hardirqs_on+0xd/0x10
[ 53.827108] [<ffffffff81023ec4>] default_idle+0x24/0x240
[ 53.827108] [<ffffffff8102377e>] arch_cpu_idle+0x2e/0x40
[ 53.827108] [<ffffffff8110371b>] cpu_startup_entry+0x2db/0x430
[ 53.827108] [<ffffffff8104e27f>] start_secondary+0x22f/0x2f0
---
kernel/auditfilter.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 570e79a..c4b89d0 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -97,8 +97,6 @@ static inline void audit_free_rule(struct audit_entry *e)
/* some rules don't have associated watches */
if (erule->watch)
audit_put_watch(erule->watch);
- if (erule->exe)
- audit_remove_mark(erule->exe);
if (erule->fields)
for (i = 0; i < erule->field_count; i++)
audit_free_lsm_field(&erule->fields[i]);
@@ -1020,6 +1018,9 @@ int audit_del_rule(struct audit_entry *entry)
if (e->rule.tree)
audit_remove_tree_rule(&e->rule);
+ if (e->rule.exe)
+ audit_remove_mark(e->rule.exe);
+
list_del_rcu(&e->list);
list_del(&e->rule.list);
call_rcu(&e->rcu, audit_free_rule_rcu);
--
1.7.1
3:25 p.m.
On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
This is a part of Peter Moody, my and Eric Paris' work to
implement
audit by executable name.
Does this patch set define an AUDIT_VERSION_SOMETHING and then set
AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel supports
it when issuing commands. Also, if its conceivable that kernels may pick and
choose what features could be backported to a curated kernel, should
AUDIT_VERSION_ be a number that is incremented or a bit mask?
-Steve
Please see the accompanying userspace patch:
https://www.redhat.com/archives/linux-audit/2014-May/msg00019.html
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:
# /usr/local/sbin/auditctl -a always,exit -F dir=/tmp -F exe=/bin/touch -F
key=touch_tmp # /usr/local/sbin/ausearch --start recent -k touch_tmp
time->Mon Jun 30 14:15:06 2014
type=CONFIG_CHANGE msg=audit(1404152106.683:149): auid=0 ses=1
subj=unconfined_u :unconfined_r:auditctl_t:s0-s0:c0.c1023 op="add_rule"
key="touch_tmp" list=4 res =1
# /usr/local/sbin/auditctl -l
-a always,exit -S all -F dir=/tmp -F exe=/bin/touch -F key=touch_tmp
# touch /tmp/test
# /usr/local/sbin/ausearch --start recent -k touch_tmp
time->Wed Jul 2 12:18:47 2014
type=UNKNOWN[1327] msg=audit(1404317927.319:132):
proctitle=746F756368002F746D702F74657374 type=PATH
msg=audit(1404317927.319:132): item=1 name="/tmp/test" inode=25997
dev=00:20 mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE type=PATH
msg=audit(1404317927.319:132): item=0 name="/tmp/" inode=11144 dev=00:20
mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0
nametype=PARENT type=CWD msg=audit(1404317927.319:132): cwd="/root"
type=SYSCALL msg=audit(1404317927.319:132): arch=c000003e syscall=2
success=yes exit=3 a0=7ffffa403dd5 a1=941 a2=1b6 a3=34b65b2c6c items=2
ppid=4321 pid=6436 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="touch_tmp"
Revision history:
v5: Revert patch "Let audit_free_rule() take care of calling
audit_remove_mark()." since it caused a group mark deadlock.
v4: Re-order and squash down fixups
Fix audit_dup_exe() to copy pathname string before calling
audit_alloc_mark().
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
Next step:
Get full-path notify working.
Eric Paris (3):
audit: implement audit by executable
audit: clean simple fsnotify implementation
audit: convert audit_exe to audit_fsnotify
Richard Guy Briggs (2):
audit: avoid double copying the audit_exe path string
Revert "fixup! audit: clean simple fsnotify implementation"
include/linux/audit.h | 1 +
include/uapi/linux/audit.h | 2 +
kernel/Makefile | 2 +-
kernel/audit.h | 39 +++++++
kernel/audit_exe.c | 49 +++++++++
kernel/audit_fsnotify.c | 237
++++++++++++++++++++++++++++++++++++++++++++ kernel/auditfilter.c |
52 +++++++++-
kernel/auditsc.c | 16 +++
8 files changed, 395 insertions(+), 3 deletions(-)
create mode 100644 kernel/audit_exe.c
create mode 100644 kernel/audit_fsnotify.c
5:47 p.m.
On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote:
On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
> This is a part of Peter Moody, my and Eric Paris' work to implement
> audit by executable name.
Does this patch set define an AUDIT_VERSION_SOMETHING and then set
AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel supports
it when issuing commands. Also, if its conceivable that kernels may pick and
choose what features could be backported to a curated kernel, should
AUDIT_VERSION_ be a number that is incremented or a bit mask?
Right now the value is 2. So this is your last hope if you want to make
it a bitmask. I'll leave that up to paul/richard to (over) design.
Support for by EXEC should probably be noted somehow. Especially since
audit_netlink_ok() sucks and return EINVAL for unknown message types. We
wouldn't need the bump to version if that returned EOPNOTSUP and
userspace could actually tell what was going on...
-Steve
> Please see the accompanying userspace patch:
> https://www.redhat.com/archives/linux-audit/2014-May/msg00019.html
> 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:
>
> # /usr/local/sbin/auditctl -a always,exit -F dir=/tmp -F exe=/bin/touch -F
> key=touch_tmp # /usr/local/sbin/ausearch --start recent -k touch_tmp
> time->Mon Jun 30 14:15:06 2014
> type=CONFIG_CHANGE msg=audit(1404152106.683:149): auid=0 ses=1
> subj=unconfined_u :unconfined_r:auditctl_t:s0-s0:c0.c1023 op="add_rule"
> key="touch_tmp" list=4 res =1
>
> # /usr/local/sbin/auditctl -l
> -a always,exit -S all -F dir=/tmp -F exe=/bin/touch -F key=touch_tmp
>
> # touch /tmp/test
>
> # /usr/local/sbin/ausearch --start recent -k touch_tmp
> time->Wed Jul 2 12:18:47 2014
> type=UNKNOWN[1327] msg=audit(1404317927.319:132):
> proctitle=746F756368002F746D702F74657374 type=PATH
> msg=audit(1404317927.319:132): item=1 name="/tmp/test" inode=25997
> dev=00:20 mode=0100644 ouid=0 ogid=0 rdev=00:00
> obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE type=PATH
> msg=audit(1404317927.319:132): item=0 name="/tmp/" inode=11144 dev=00:20
> mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0
> nametype=PARENT type=CWD msg=audit(1404317927.319:132): cwd="/root"
> type=SYSCALL msg=audit(1404317927.319:132): arch=c000003e syscall=2
> success=yes exit=3 a0=7ffffa403dd5 a1=941 a2=1b6 a3=34b65b2c6c items=2
> ppid=4321 pid=6436 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="touch_tmp"
>
>
> Revision history:
> v5: Revert patch "Let audit_free_rule() take care of calling
> audit_remove_mark()." since it caused a group mark deadlock.
>
> v4: Re-order and squash down fixups
> Fix audit_dup_exe() to copy pathname string before calling
> audit_alloc_mark().
>
> 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
>
>
> Next step:
> Get full-path notify working.
>
>
> Eric Paris (3):
> audit: implement audit by executable
> audit: clean simple fsnotify implementation
> audit: convert audit_exe to audit_fsnotify
>
> Richard Guy Briggs (2):
> audit: avoid double copying the audit_exe path string
> Revert "fixup! audit: clean simple fsnotify implementation"
>
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 2 +
> kernel/Makefile | 2 +-
> kernel/audit.h | 39 +++++++
> kernel/audit_exe.c | 49 +++++++++
> kernel/audit_fsnotify.c | 237
> ++++++++++++++++++++++++++++++++++++++++++++ kernel/auditfilter.c |
> 52 +++++++++-
> kernel/auditsc.c | 16 +++
> 8 files changed, 395 insertions(+), 3 deletions(-)
> create mode 100644 kernel/audit_exe.c
> create mode 100644 kernel/audit_fsnotify.c
6:02 p.m.
On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote:
On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote:
> On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
> > This is a part of Peter Moody, my and Eric Paris' work to implement
> > audit by executable name.
>
> Does this patch set define an AUDIT_VERSION_SOMETHING and then set
> AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel
> supports it when issuing commands. Also, if its conceivable that kernels
> may pick and choose what features could be backported to a curated
> kernel, should AUDIT_VERSION_ be a number that is incremented or a bit
> mask?
Right now the value is 2. So this is your last hope if you want to make
it a bitmask. I'll leave that up to paul/richard to (over) design.
Audit is nothing if not over-designed. I want to make sure we're consistent
with the previous design methodologies ;)
I've been thinking about this for about the past half-hour while I've been
going through some other mail and I'm not really enthused about using the
version number to encode capabilities. What sort of problems would we have if
we introduced a new audit netlink command to query the kernel for audit
capabilities?
--
paul moore
security and virtualization @ redhat
6:33 p.m.
On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote:
On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote:
> On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote:
> > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
> > > This is a part of Peter Moody, my and Eric Paris' work to implement
> > > audit by executable name.
> >
> > Does this patch set define an AUDIT_VERSION_SOMETHING and then set
> > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel
> > supports it when issuing commands. Also, if its conceivable that kernels
> > may pick and choose what features could be backported to a curated
> > kernel, should AUDIT_VERSION_ be a number that is incremented or a bit
> > mask?
>
> Right now the value is 2. So this is your last hope if you want to make
> it a bitmask. I'll leave that up to paul/richard to (over) design.
Audit is nothing if not over-designed. I want to make sure we're consistent
with the previous design methodologies ;)
I've been thinking about this for about the past half-hour while I've been
going through some other mail and I'm not really enthused about using the
version number to encode capabilities. What sort of problems would we have
if we introduced a new audit netlink command to query the kernel for audit
capabilities?
I thought that is what we were getting in this patch:
https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html
As I understood it, I send an AUDIT_GET command on netlink and then look in
status.version to see what we have. I really think that in the mainline
kernel, there will be a steady increment of capabilities. However, for
distributions, they may want to pick and choose which capabilities to backport
to their shipping kernel. Meaning in practice, a bitmap may be better to allow
cherry picking capabilities and user space being able to make informed
decisions.
I really don't mind if this is done by a new netlink command (but if we do,
what happens to status.version?) or if we just keep going with status.version.
Just tell me which it is.
-Steve
6:49 p.m.
On Monday, October 20, 2014 07:33:39 PM Steve Grubb wrote:
On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote:
> On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote:
> > On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote:
> > > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
> > > > This is a part of Peter Moody, my and Eric Paris' work to
implement
> > > > audit by executable name.
> > >
> > > Does this patch set define an AUDIT_VERSION_SOMETHING and then set
> > > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel
> > > supports it when issuing commands. Also, if its conceivable that
> > > kernels
> > > may pick and choose what features could be backported to a curated
> > > kernel, should AUDIT_VERSION_ be a number that is incremented or a bit
> > > mask?
> >
> > Right now the value is 2. So this is your last hope if you want to make
> > it a bitmask. I'll leave that up to paul/richard to (over) design.
>
> Audit is nothing if not over-designed. I want to make sure we're
> consistent with the previous design methodologies ;)
>
> I've been thinking about this for about the past half-hour while I've been
> going through some other mail and I'm not really enthused about using the
> version number to encode capabilities. What sort of problems would we
> have
> if we introduced a new audit netlink command to query the kernel for audit
> capabilities?
I thought that is what we were getting in this patch:
https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html
As I understood it, I send an AUDIT_GET command on netlink and then look in
status.version to see what we have. I really think that in the mainline
kernel, there will be a steady increment of capabilities. However, for
distributions, they may want to pick and choose which capabilities to
backport to their shipping kernel. Meaning in practice, a bitmap may be
better to allow cherry picking capabilities and user space being able to
make informed decisions.
I really don't mind if this is done by a new netlink command (but if we do,
what happens to status.version?) or if we just keep going with
status.version. Just tell me which it is.
Further to the point of status.version, its declared as a __u32. So if it were
a bit map, we can have 32 different features userspace needs to make support
decisions on. I have a feeling that will last many years because I really
can't see audit gaining too many more capabilities.
-Steve
4:56 p.m.
On Monday, October 20, 2014 07:33:39 PM Steve Grubb wrote:
On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote:
> On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote:
> > On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote:
> > > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
> > > > This is a part of Peter Moody, my and Eric Paris' work to
implement
> > > > audit by executable name.
> > >
> > > Does this patch set define an AUDIT_VERSION_SOMETHING and then set
> > > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel
> > > supports it when issuing commands. Also, if its conceivable that
> > > kernels
> > > may pick and choose what features could be backported to a curated
> > > kernel, should AUDIT_VERSION_ be a number that is incremented or a bit
> > > mask?
> >
> > Right now the value is 2. So this is your last hope if you want to make
> > it a bitmask. I'll leave that up to paul/richard to (over) design.
>
> Audit is nothing if not over-designed. I want to make sure we're
> consistent with the previous design methodologies ;)
>
> I've been thinking about this for about the past half-hour while I've been
> going through some other mail and I'm not really enthused about using the
> version number to encode capabilities. What sort of problems would we
> have if we introduced a new audit netlink command to query the kernel for
> audit capabilities?
I thought that is what we were getting in this patch:
https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html
That patch, and the simple name "version", looks more like a version number
and not a capabilities bitmap. However, as Eric previously pointed out, since
we are only at version 2, all is not lost.
As I understood it, I send an AUDIT_GET command on netlink and then
look in
status.version to see what we have. I really think that in the mainline
kernel, there will be a steady increment of capabilities. However, for
distributions, they may want to pick and choose which capabilities to
backport to their shipping kernel. Meaning in practice, a bitmap may be
better to allow cherry picking capabilities and user space being able to
make informed decisions.
If we are intending to use this as a way of checking for functionality then it
really should be a bitmap and not a version number. Regardless of if we are
talking about an upstream or distribution kernel.
I really don't mind if this is done by a new netlink command (but
if we do,
what happens to status.version?) or if we just keep going with
status.version. Just tell me which it is.
No, let's stick with what we have now. I mistakenly assumed that since the
struct field and userspace #defines included "version" that the value was
actually a version number ... silly me, I have no idea why I thought that.
So, with this in mind, I think a couple of small tweaks are in order (sorry
Richard), in no particular order:
* Change the audit_status.version field comment in include/uapi/linux/audit.h
to "/* audit functionality bitmap */", or similar. We can't really change
the
structure now, but the comment is fair game.
* Change AUDIT_VERSION_LATEST to a bitmask instead of a number. For example,
it should be 3 given the current code, not 2. In a perfect world this
wouldn't even be in the uapi header, but it is so we need to keep it updated.
Bumping it higher should be backwards compatible.
Can anyone think of anything else that might be affected by this?
--
paul moore
security and virtualization @ redhat
5:06 p.m.
On Tuesday, October 21, 2014 05:56:36 PM Paul Moore wrote:
On Monday, October 20, 2014 07:33:39 PM Steve Grubb wrote:
> On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote:
> > On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote:
> > > On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote:
> > > > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote:
> > > > > This is a part of Peter Moody, my and Eric Paris' work to
> > > > > implement
> > > > > audit by executable name.
> > > >
> > > > Does this patch set define an AUDIT_VERSION_SOMETHING and then set
> > > > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel
> > > > supports it when issuing commands. Also, if its conceivable that
> > > > kernels
> > > > may pick and choose what features could be backported to a curated
> > > > kernel, should AUDIT_VERSION_ be a number that is incremented or a
> > > > bit
> > > > mask?
> > >
> > > Right now the value is 2. So this is your last hope if you want to
> > > make
> > > it a bitmask. I'll leave that up to paul/richard to (over) design.
> >
> > Audit is nothing if not over-designed. I want to make sure we're
> > consistent with the previous design methodologies ;)
> >
> > I've been thinking about this for about the past half-hour while I've
> > been
> > going through some other mail and I'm not really enthused about using
> > the
> > version number to encode capabilities. What sort of problems would we
> > have if we introduced a new audit netlink command to query the kernel
> > for
> > audit capabilities?
>
> I thought that is what we were getting in this patch:
> https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html
That patch, and the simple name "version", looks more like a version number
and not a capabilities bitmap. However, as Eric previously pointed out,
since we are only at version 2, all is not lost.
> As I understood it, I send an AUDIT_GET command on netlink and then look
> in
> status.version to see what we have. I really think that in the mainline
> kernel, there will be a steady increment of capabilities. However, for
> distributions, they may want to pick and choose which capabilities to
> backport to their shipping kernel. Meaning in practice, a bitmap may be
> better to allow cherry picking capabilities and user space being able to
> make informed decisions.
If we are intending to use this as a way of checking for functionality then
it really should be a bitmap and not a version number. Regardless of if we
are talking about an upstream or distribution kernel.
> I really don't mind if this is done by a new netlink command (but if we
> do,
> what happens to status.version?) or if we just keep going with
> status.version. Just tell me which it is.
No, let's stick with what we have now. I mistakenly assumed that since the
struct field and userspace #defines included "version" that the value was
actually a version number ... silly me, I have no idea why I thought that.
So, with this in mind, I think a couple of small tweaks are in order (sorry
Richard), in no particular order:
* Change the audit_status.version field comment in
include/uapi/linux/audit.h to "/* audit functionality bitmap */", or
similar. We can't really change the structure now, but the comment is fair
game.
* Change AUDIT_VERSION_LATEST to a bitmask instead of a number. For
example, it should be 3 given the current code, not 2. In a perfect world
this wouldn't even be in the uapi header, but it is so we need to keep it
updated. Bumping it higher should be backwards compatible.
Can anyone think of anything else that might be affected by this?
This plan sounds good to me.
Thanks,
-Steve
5:19 p.m.
On Tue, 2014-10-21 at 17:56 -0400, Paul Moore wrote:
* Change the audit_status.version field comment in
include/uapi/linux/audit.h
to "/* audit functionality bitmap */", or similar. We can't really change
the
structure now, but the comment is fair game.
Trying to think how to do things with a #define so you can rename,
"version" is pretty darn generic to pre-process. You could make it a
union, so userspace code and use a sane name....
* Change AUDIT_VERSION_LATEST to a bitmask instead of a number. For example,
it should be 3 given the current code, not 2. In a perfect world this
wouldn't even be in the uapi header, but it is so we need to keep it updated.
Bumping it higher should be backwards compatible.
Getting 1 without 2 is actually hard to accompish as I remember, but
yes, you're right, i missed that. I should be 3....
Can anyone think of anything else that might be affected by this?
No one uses this stuff, just change it.
5:35 p.m.
On Tuesday, October 21, 2014 06:19:52 PM Eric Paris wrote:
On Tue, 2014-10-21 at 17:56 -0400, Paul Moore wrote:
> * Change the audit_status.version field comment in
> include/uapi/linux/audit.h to "/* audit functionality bitmap */", or
> similar. We can't really change the structure now, but the comment is
> fair game.
Trying to think how to do things with a #define so you can rename,
"version" is pretty darn generic to pre-process. You could make it a
union, so userspace code and use a sane name....
Yeah, I thought about suggesting the #define approach but figured that might
just be me worrying about the color of the paint ... okay, Richard, why don't
you go ahead and change the version field name and put in a #define for
compatibility.
> Can anyone think of anything else that might be affected by
this?
No one uses this stuff, just change it.
Yes, but I feel like I need to at least ask the question; how much attention I
pay to the answers is something else ...
--
paul moore
security and virtualization @ redhat
2:48 p.m.
On 14/10/21, Paul Moore wrote:
On Tuesday, October 21, 2014 06:19:52 PM Eric Paris wrote:
> On Tue, 2014-10-21 at 17:56 -0400, Paul Moore wrote:
> > * Change the audit_status.version field comment in
> > include/uapi/linux/audit.h to "/* audit functionality bitmap */", or
> > similar. We can't really change the structure now, but the comment is
> > fair game.
>
> Trying to think how to do things with a #define so you can rename,
> "version" is pretty darn generic to pre-process. You could make it a
> union, so userspace code and use a sane name....
Yeah, I thought about suggesting the #define approach but figured that might
just be me worrying about the color of the paint ... okay, Richard, why don't
you go ahead and change the version field name and put in a #define for
compatibility.
The #define is a nice way to work around backward compatibility.
> > Can anyone think of anything else that might be affected by
this?
>
> No one uses this stuff, just change it.
Yes, but I feel like I need to at least ask the question; how much attention I
pay to the answers is something else ...
I'm still skeptical this won't blow up... Like the capabilities bitmap
did. I suspect there isn't agreement on what constitutes a feature. We
just added a set/get features bitmap a year ago for things to be turned
on/off and locked... How does this features bitmap fit in with that
features config?
I don't disagree that a bitmap would be more useful for various
distributions to pick and choose that which they choose to support over
a version number that won't tell the whole story.
paul moore
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
3:05 p.m.
On Wednesday, October 29, 2014 03:48:40 PM Richard Guy Briggs wrote:
On 14/10/21, Paul Moore wrote:
> > > Can anyone think of anything else that might be affected by this?
> >
> > No one uses this stuff, just change it.
>
> Yes, but I feel like I need to at least ask the question; how much
> attention I pay to the answers is something else ...
I'm still skeptical this won't blow up... Like the capabilities bitmap
did. I suspect there isn't agreement on what constitutes a feature.
Anything major that user space would have to know about to determine if its
supported. If you don't know, just ask if we need to add a bit to the bitmap.
Some examples, adding the object comparison engine, adding the loginuid-
immutable feature, if we added filtering on TTY that would also qualify (not
asking for that). Otherwise, user space get EINVAL on the netlink operation
which is not useful in explaining why the command was rejected.
We just added a set/get features bitmap a year ago for things to be
turned
on/off and locked... How does this features bitmap fit in with that
features config?
I think of that as commanding the features, not determining if they exist.
I don't disagree that a bitmap would be more useful for various
distributions to pick and choose that which they choose to support over
a version number that won't tell the whole story.
I also can be used to allow deprecation in a controlled way such that helpful
messages are given to the system admin.
-Steve
4:54 p.m.
On 14/10/29, Steve Grubb wrote:
On Wednesday, October 29, 2014 03:48:40 PM Richard Guy Briggs wrote:
> On 14/10/21, Paul Moore wrote:
> > > > Can anyone think of anything else that might be affected by this?
> > >
> > > No one uses this stuff, just change it.
> >
> > Yes, but I feel like I need to at least ask the question; how much
> > attention I pay to the answers is something else ...
>
> I'm still skeptical this won't blow up... Like the capabilities bitmap
> did. I suspect there isn't agreement on what constitutes a feature.
Anything major that user space would have to know about to determine if its
supported. If you don't know, just ask if we need to add a bit to the bitmap.
Some examples, adding the object comparison engine, adding the loginuid-
immutable feature, if we added filtering on TTY that would also qualify (not
asking for that). Otherwise, user space get EINVAL on the netlink operation
which is not useful in explaining why the command was rejected.
Well, I guess this falls under Linus' "thou shalt not break userspace",
but it would certainly be tempting to change some of those to
EOPNOTSUPP.
> We just added a set/get features bitmap a year ago for things to
be turned
> on/off and locked... How does this features bitmap fit in with that
> features config?
I think of that as commanding the features, not determining if they exist.
Which partly addresses another thing that occured to me which was that
there could be overlap between the two. status.version will have more
capacity due to only one bit needed per feature.
> I don't disagree that a bitmap would be more useful for
various
> distributions to pick and choose that which they choose to support over
> a version number that won't tell the whole story.
I also can be used to allow deprecation in a controlled way such that helpful
messages are given to the system admin.
That would work only for new things added, enabled explicitly with that
bit set in the bitfield.
-Steve
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
6:59 p.m.
On Wed, 2014-10-29 at 17:54 -0400, Richard Guy Briggs wrote:
On 14/10/29, Steve Grubb wrote:
> On Wednesday, October 29, 2014 03:48:40 PM Richard Guy Briggs wrote:
> > On 14/10/21, Paul Moore wrote:
> > > > > Can anyone think of anything else that might be affected by
this?
> > > >
> > > > No one uses this stuff, just change it.
> > >
> > > Yes, but I feel like I need to at least ask the question; how much
> > > attention I pay to the answers is something else ...
> >
> > I'm still skeptical this won't blow up... Like the capabilities
bitmap
> > did. I suspect there isn't agreement on what constitutes a feature.
>
> Anything major that user space would have to know about to determine if its
> supported. If you don't know, just ask if we need to add a bit to the bitmap.
> Some examples, adding the object comparison engine, adding the loginuid-
> immutable feature, if we added filtering on TTY that would also qualify (not
> asking for that). Otherwise, user space get EINVAL on the netlink operation
> which is not useful in explaining why the command was rejected.
Well, I guess this falls under Linus' "thou shalt not break userspace",
but it would certainly be tempting to change some of those to
EOPNOTSUPP.
You only break userspace if something breaks :)
8:17 p.m.
On 14/10/29, Eric Paris wrote:
On Wed, 2014-10-29 at 17:54 -0400, Richard Guy Briggs wrote:
> On 14/10/29, Steve Grubb wrote:
> > On Wednesday, October 29, 2014 03:48:40 PM Richard Guy Briggs wrote:
> > > On 14/10/21, Paul Moore wrote:
> > > > > > Can anyone think of anything else that might be affected by
this?
> > > > >
> > > > > No one uses this stuff, just change it.
> > > >
> > > > Yes, but I feel like I need to at least ask the question; how much
> > > > attention I pay to the answers is something else ...
> > >
> > > I'm still skeptical this won't blow up... Like the capabilities
bitmap
> > > did. I suspect there isn't agreement on what constitutes a feature.
> >
> > Anything major that user space would have to know about to determine if its
> > supported. If you don't know, just ask if we need to add a bit to the
bitmap.
> > Some examples, adding the object comparison engine, adding the loginuid-
> > immutable feature, if we added filtering on TTY that would also qualify (not
> > asking for that). Otherwise, user space get EINVAL on the netlink operation
> > which is not useful in explaining why the command was rejected.
>
> Well, I guess this falls under Linus' "thou shalt not break
userspace",
> but it would certainly be tempting to change some of those to
> EOPNOTSUPP.
You only break userspace if something breaks :)
So which scratch monkey do we mount before sending it upstream?
We saw how actually allowing CAP_AUDIT_WRITE from non-init PID
namespaces backfired on us in stuff which didn't use audit before...
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
3705
days inactive
3732
days old
linux-audit@lists.linux-audit.osci.io
19 comments
4 participants
participants (4)
-
Eric Paris -
Paul Moore -
Richard Guy Briggs -
Steve Grubb