[PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
startup:
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load

This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.

Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().

Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.

Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry. Even if one is found, there may be more than
one mount point. At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.

Sample output:
type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"

See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42

Signed-off-by: Richard Guy Briggs <***@redhat.com>

---
v3:
fix audit_buffer leak and dname error allocation leak audit_log_name
only put audit_name->dentry if it is being replaced

v2:
minor cosmetic changes and support fs filter patch
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 19 +++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2150bdc..1ef4ec8 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -240,7 +240,7 @@ extern void __audit_inode(struct filename *name, const struct dentry *dentry,
unsigned int flags);
extern void __audit_file(const struct file *);
extern void __audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type);
extern void __audit_seccomp(unsigned long syscall, long signr, int code);
extern void __audit_ptrace(struct task_struct *t);
@@ -305,7 +305,7 @@ static inline void audit_inode_parent_hidden(struct filename *name,
AUDIT_INODE_PARENT | AUDIT_INODE_HIDDEN);
}
static inline void audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type) {
if (unlikely(!audit_dummy_context()))
__audit_inode_child(parent, dentry, type);
@@ -486,7 +486,7 @@ static inline void __audit_inode(struct filename *name,
unsigned int flags)
{ }
static inline void __audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type)
{ }
static inline void audit_inode(struct filename *name,
@@ -500,7 +500,7 @@ static inline void audit_inode_parent_hidden(struct filename *name,
const struct dentry *dentry)
{ }
static inline void audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type)
{ }
static inline void audit_core_dumps(long signr)
diff --git a/kernel/audit.c b/kernel/audit.c
index 59e60e0..d6e6e4e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -72,6 +72,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>

#include "audit.h"

@@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
name->gid = inode->i_gid;
name->rdev = inode->i_rdev;
security_inode_getsecid(inode, &name->osid);
+ if (name->dentry) {
+ dput(name->dentry);
+ name->dentry = NULL;
+ }
audit_copy_fcaps(name, dentry);
}

@@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
audit_log_n_untrustedstring(ab, n->name->name,
n->name_len);
}
+ } else if (n->dentry) {
+ char *fullpath;
+ const char *fullpathp = NULL;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (fullpath)
+ fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
+ if (IS_ERR(fullpathp)) {
+ fullpathp = NULL;
+ kfree(fullpath);
+ }
+ audit_log_format(ab, " name=%s(0x%lx):%s",
+ n->dentry->d_sb->s_type->name ?: "?",
+ n->dentry->d_sb->s_magic, fullpathp ?: "?");
} else
audit_log_format(ab, " name=(null)");

diff --git a/kernel/audit.h b/kernel/audit.h
index b331d9b..c01defb 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -85,6 +85,7 @@ struct audit_names {

unsigned long ino;
dev_t dev;
+ struct dentry *dentry;
umode_t mode;
kuid_t uid;
kgid_t gid;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4a42db5..11848df 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -74,6 +74,7 @@
#include <linux/string.h>
#include <linux/uaccess.h>
#include <uapi/linux/limits.h>
+#include <linux/dcache.h>

#include "audit.h"

@@ -881,6 +882,8 @@ static inline void audit_free_names(struct audit_context *context)
list_del(&n->list);
if (n->name)
putname(n->name);
+ if (n->dentry)
+ dput(n->dentry);
if (n->should_free)
kfree(n);
}
@@ -1861,7 +1864,7 @@ void __audit_file(const struct file *file)
* unsuccessful attempts.
*/
void __audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type)
{
struct audit_context *context = current->audit_context;
@@ -1917,6 +1920,7 @@ void __audit_inode_child(struct inode *parent,
if (!n)
return;
audit_copy_inode(n, NULL, parent);
+ n->dentry = dget_parent(dentry);
}

if (!found_child) {
@@ -1938,6 +1942,8 @@ void __audit_inode_child(struct inode *parent,
audit_copy_inode(found_child, dentry, inode);
else
found_child->ino = AUDIT_INO_UNSET;
+ if (!found_parent)
+ found_child->dentry = dget(dentry);
}
EXPORT_SYMBOL_GPL(__audit_inode_child);

[Richard Guy Briggs]

Tracefs or debugfs were causing hundreds to thousands of PATH records to
be associated with the init_module and finit_module SYSCALL records on a
few modules when the following rule was in place for startup:
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load

Provide a method to ignore these large number of PATH records from
overwhelming the logs if they are not of interest. Introduce a new
filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
which keys off the filesystem 4-octet hexadecimal magic identifier to
filter specific filesystem PATH records.

An example rule would look like:
-a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
-a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs

Arguably the better way to address this issue is to disable tracefs and
debugfs on boot from production systems.

See: https://github.com/linux-audit/audit-kernel/issues/16
See: https://github.com/linux-audit/audit-userspace/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42

Signed-off-by: Richard Guy Briggs <***@redhat.com>

---
v3: rebase
v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
---
include/uapi/linux/audit.h | 8 ++++++--
kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
kernel/auditsc.c | 23 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..be71134 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -155,8 +155,9 @@
#define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
#define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
#define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
+#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */

-#define AUDIT_NR_FILTERS 6
+#define AUDIT_NR_FILTERS 7

#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */

@@ -256,6 +257,7 @@
#define AUDIT_OBJ_LEV_HIGH 23
#define AUDIT_LOGINUID_SET 24
#define AUDIT_SESSIONID 25 /* Session ID */
+#define AUDIT_FSTYPE 26 /* FileSystem Type */

/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
@@ -335,13 +337,15 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040

#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_FS)

/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa58..4a1758a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[3]),
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
-#if AUDIT_NR_FILTERS != 6
+ LIST_HEAD_INIT(audit_filter_list[6]),
+#if AUDIT_NR_FILTERS != 7
#error Fix audit_filter_list initialiser
#endif
};
@@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[3]),
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
+ LIST_HEAD_INIT(audit_rules_list[6]),
};

DEFINE_MUTEX(audit_filter_mutex);
@@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
#endif
case AUDIT_FILTER_USER:
case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_FS:
;
}
if (unlikely(rule->action == AUDIT_POSSIBLE)) {
@@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
entry->rule.listnr != AUDIT_FILTER_USER)
return -EINVAL;
break;
+ case AUDIT_FSTYPE:
+ if (entry->rule.listnr != AUDIT_FILTER_FS)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_FS:
+ switch(f->type) {
+ case AUDIT_FSTYPE:
+ case AUDIT_FILTERKEY:
+ break;
+ default:
+ return -EINVAL;
+ }
}

switch(f->type) {
@@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
/* FALL THROUGH */
case AUDIT_ARCH:
+ case AUDIT_FSTYPE:
if (f->op != Audit_not_equal && f->op != Audit_equal)
return -EINVAL;
break;
@@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;

- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_USER:
+ case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_FS:
dont_count = 1;
+ }
#endif

mutex_lock(&audit_filter_mutex);
@@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;

- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_USER:
+ case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_FS:
dont_count = 1;
+ }
#endif

mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 11848df..ce6cbda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
struct inode *inode = d_backing_inode(dentry);
const char *dname = dentry->d_name.name;
struct audit_names *n, *found_parent = NULL, *found_child = NULL;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;

if (!context->in_syscall)
return;

+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE) {
+ if (audit_comparator(parent->i_sb->s_magic,
+ f->op, f->val)) {
+ if (e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
+ }
+ }
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (inode)
handle_one(inode);

[Paul Moore]

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of PATH records to
be associated with the init_module and finit_module SYSCALL records on a
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
Provide a method to ignore these large number of PATH records from
overwhelming the logs if they are not of interest. Introduce a new
filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
which keys off the filesystem 4-octet hexadecimal magic identifier to
filter specific filesystem PATH records.
-a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
-a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
Arguably the better way to address this issue is to disable tracefs and
debugfs on boot from production systems.
See: https://github.com/linux-audit/audit-kernel/issues/16
See: https://github.com/linux-audit/audit-userspace/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
v3: rebase
v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
---
include/uapi/linux/audit.h | 8 ++++++--
kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
kernel/auditsc.c | 23 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 9 deletions(-)

Both this patch and 1/2 look okay, but I'm not going to merge either
until after the merge window closes.

Steve, I assume you are still happy with the kernel/userspace
interface for this?

Post by Richard Guy Briggs
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..be71134 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -155,8 +155,9 @@
#define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
#define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
#define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
+#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
-#define AUDIT_NR_FILTERS 6
+#define AUDIT_NR_FILTERS 7
#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
@@ -256,6 +257,7 @@
#define AUDIT_OBJ_LEV_HIGH 23
#define AUDIT_LOGINUID_SET 24
#define AUDIT_SESSIONID 25 /* Session ID */
+#define AUDIT_FSTYPE 26 /* FileSystem Type */
/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
@@ -335,13 +337,15 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_FS)
/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa58..4a1758a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[3]),
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
-#if AUDIT_NR_FILTERS != 6
+ LIST_HEAD_INIT(audit_filter_list[6]),
+#if AUDIT_NR_FILTERS != 7
#error Fix audit_filter_list initialiser
#endif
};
@@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[3]),
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
+ LIST_HEAD_INIT(audit_rules_list[6]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
#endif
;
}
if (unlikely(rule->action == AUDIT_POSSIBLE)) {
@@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
entry->rule.listnr != AUDIT_FILTER_USER)
return -EINVAL;
break;
+ if (entry->rule.listnr != AUDIT_FILTER_FS)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ switch(f->type) {
+ break;
+ return -EINVAL;
+ }
}
switch(f->type) {
@@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
/* FALL THROUGH */
if (f->op != Audit_not_equal && f->op != Audit_equal)
return -EINVAL;
break;
@@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
@@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 11848df..ce6cbda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
struct inode *inode = d_backing_inode(dentry);
const char *dname = dentry->d_name.name;
struct audit_names *n, *found_parent = NULL, *found_child = NULL;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;
if (!context->in_syscall)
return;
+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE) {
+ if (audit_comparator(parent->i_sb->s_magic,
+ f->op, f->val)) {
+ if (e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
+ }
+ }
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (inode)
handle_one(inode);
--
1.7.1
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

[Steven Rostedt]

On Thu, 7 Sep 2017 18:36:32 -0400

Post by Paul Moore
Steve, I assume you are still happy with the kernel/userspace
interface for this?

I've been working on a lot of different things lately, and this has
totally been flushed out of my memory cache. What was the change that
this is making with regard to kernel/userspace and tracefs?

Thanks!

-- Steve

[Steven Rostedt]

On Thu, 7 Sep 2017 19:05:37 -0400

My question was aimed at Steve Grubb, not you. Sorry for the
confusion, wrong Steve :)

Great good to hear. I thought I was having a senior moment.

-- Steve

[Steve Grubb]

Post by Paul Moore

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of PATH records to
be associated with the init_module and finit_module SYSCALL records on a
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
Provide a method to ignore these large number of PATH records from
overwhelming the logs if they are not of interest. Introduce a new
filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
which keys off the filesystem 4-octet hexadecimal magic identifier to
filter specific filesystem PATH records.
-a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
-a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
Arguably the better way to address this issue is to disable tracefs and
debugfs on boot from production systems.
See: https://github.com/linux-audit/audit-kernel/issues/16
See: https://github.com/linux-audit/audit-userspace/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
v3: rebase
v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
---
include/uapi/linux/audit.h | 8 ++++++--
kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
kernel/auditsc.c | 23 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 9 deletions(-)

Both this patch and 1/2 look okay, but I'm not going to merge either
until after the merge window closes.
Steve, I assume you are still happy with the kernel/userspace
interface for this?

The user space portion was applied a week or two ago. So, I guess proceed with
merging this.

-Steve

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..be71134 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -155,8 +155,9 @@
#define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system
watches */ #define AUDIT_FILTER_EXIT 0x04 /* Apply rule at
syscall exit */ #define AUDIT_FILTER_TYPE 0x05 /* Apply rule at
audit_log_start */>
+#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
-#define AUDIT_NR_FILTERS 6
+#define AUDIT_NR_FILTERS 7
#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
@@ -256,6 +257,7 @@
#define AUDIT_OBJ_LEV_HIGH 23
#define AUDIT_LOGINUID_SET 24
#define AUDIT_SESSIONID 25 /* Session ID */
+#define AUDIT_FSTYPE 26 /* FileSystem Type */
/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT).
*/
@@ -335,13 +337,15 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME |
\
AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
AUDIT_FEATURE_BITMAP_SESSIONID_FILTER |
\
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_FS)
/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa58..4a1758a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[3]),
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
-#if AUDIT_NR_FILTERS != 6
+ LIST_HEAD_INIT(audit_filter_list[6]),
+#if AUDIT_NR_FILTERS != 7
#error Fix audit_filter_list initialiser
#endif
};
@@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[3]),
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
+ LIST_HEAD_INIT(audit_rules_list[6]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry
*entry)>
#endif
;
}
if (unlikely(rule->action == AUDIT_POSSIBLE)) {
@@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry
*entry, struct audit_field *f)>
entry->rule.listnr != AUDIT_FILTER_USER)
return -EINVAL;
break;
+ if (entry->rule.listnr != AUDIT_FILTER_FS)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ switch(f->type) {
+ break;
+ return -EINVAL;
+ }
}
switch(f->type) {
@@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry
*entry, struct audit_field *f)>
return -EINVAL;
/* FALL THROUGH */
if (f->op != Audit_not_equal && f->op != Audit_equal)
return -EINVAL;
break;
@@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry
*entry)>
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
@@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 11848df..ce6cbda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
struct inode *inode = d_backing_inode(dentry);
const char *dname = dentry->d_name.name;
struct audit_names *n, *found_parent = NULL, *found_child = NULL;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;
if (!context->in_syscall)
return;
+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f =
&e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE) {
+ if
(audit_comparator(parent->i_sb->s_magic, +
f->op, f->val)) {
+ if (e->rule.action ==
AUDIT_NEVER) { +
rcu_read_unlock(); +
return;
+ }
+ }
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (inode)
handle_one(inode);
--
1.7.1
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

[Paul Moore]

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of PATH records to
be associated with the init_module and finit_module SYSCALL records on a
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
Provide a method to ignore these large number of PATH records from
overwhelming the logs if they are not of interest. Introduce a new
filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
which keys off the filesystem 4-octet hexadecimal magic identifier to
filter specific filesystem PATH records.
-a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
-a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
Arguably the better way to address this issue is to disable tracefs and
debugfs on boot from production systems.
See: https://github.com/linux-audit/audit-kernel/issues/16
See: https://github.com/linux-audit/audit-userspace/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
v3: rebase
v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
---
include/uapi/linux/audit.h | 8 ++++++--
kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
kernel/auditsc.c | 23 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 9 deletions(-)

I'm in the process of applying this patch right now, and I'm seeing a
lot of space/tab whitespace damage in this patch. I'll fix it up, but
please be more careful in the future.

Post by Richard Guy Briggs
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..be71134 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -155,8 +155,9 @@
#define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
#define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
#define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
+#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
-#define AUDIT_NR_FILTERS 6
+#define AUDIT_NR_FILTERS 7
#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
@@ -256,6 +257,7 @@
#define AUDIT_OBJ_LEV_HIGH 23
#define AUDIT_LOGINUID_SET 24
#define AUDIT_SESSIONID 25 /* Session ID */
+#define AUDIT_FSTYPE 26 /* FileSystem Type */
/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
@@ -335,13 +337,15 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_FS)
/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa58..4a1758a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[3]),
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
-#if AUDIT_NR_FILTERS != 6
+ LIST_HEAD_INIT(audit_filter_list[6]),
+#if AUDIT_NR_FILTERS != 7
#error Fix audit_filter_list initialiser
#endif
};
@@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[3]),
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
+ LIST_HEAD_INIT(audit_rules_list[6]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
#endif
;
}
if (unlikely(rule->action == AUDIT_POSSIBLE)) {
@@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
entry->rule.listnr != AUDIT_FILTER_USER)
return -EINVAL;
break;
+ if (entry->rule.listnr != AUDIT_FILTER_FS)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ switch(f->type) {
+ break;
+ return -EINVAL;
+ }
}
switch(f->type) {
@@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
/* FALL THROUGH */
if (f->op != Audit_not_equal && f->op != Audit_equal)
return -EINVAL;
break;
@@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
@@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 11848df..ce6cbda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
struct inode *inode = d_backing_inode(dentry);
const char *dname = dentry->d_name.name;
struct audit_names *n, *found_parent = NULL, *found_child = NULL;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;
if (!context->in_syscall)
return;
+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE) {
+ if (audit_comparator(parent->i_sb->s_magic,
+ f->op, f->val)) {
+ if (e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
+ }
+ }
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (inode)
handle_one(inode);
--
1.7.1
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of PATH records to
be associated with the init_module and finit_module SYSCALL records on a
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
Provide a method to ignore these large number of PATH records from
overwhelming the logs if they are not of interest. Introduce a new
filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
which keys off the filesystem 4-octet hexadecimal magic identifier to
filter specific filesystem PATH records.
-a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
-a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
Arguably the better way to address this issue is to disable tracefs and
debugfs on boot from production systems.
See: https://github.com/linux-audit/audit-kernel/issues/16
See: https://github.com/linux-audit/audit-userspace/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
v3: rebase
v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
---
include/uapi/linux/audit.h | 8 ++++++--
kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
kernel/auditsc.c | 23 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 9 deletions(-)

I'm in the process of applying this patch right now, and I'm seeing a
lot of space/tab whitespace damage in this patch. I'll fix it up, but
please be more careful in the future.

Ack, I see them now, must have been a mouse copy/paste rather than the
safer editor/file copy/paste and obviously missed the checkpatch step.
Sorry for that. Thanks for cleaning it up.

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..be71134 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -155,8 +155,9 @@
#define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
#define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
#define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
+#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
-#define AUDIT_NR_FILTERS 6
+#define AUDIT_NR_FILTERS 7
#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
@@ -256,6 +257,7 @@
#define AUDIT_OBJ_LEV_HIGH 23
#define AUDIT_LOGINUID_SET 24
#define AUDIT_SESSIONID 25 /* Session ID */
+#define AUDIT_FSTYPE 26 /* FileSystem Type */
/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
@@ -335,13 +337,15 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_FS)
/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa58..4a1758a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[3]),
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
-#if AUDIT_NR_FILTERS != 6
+ LIST_HEAD_INIT(audit_filter_list[6]),
+#if AUDIT_NR_FILTERS != 7
#error Fix audit_filter_list initialiser
#endif
};
@@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[3]),
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
+ LIST_HEAD_INIT(audit_rules_list[6]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
#endif
;
}
if (unlikely(rule->action == AUDIT_POSSIBLE)) {
@@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
entry->rule.listnr != AUDIT_FILTER_USER)
return -EINVAL;
break;
+ if (entry->rule.listnr != AUDIT_FILTER_FS)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ switch(f->type) {
+ break;
+ return -EINVAL;
+ }
}
switch(f->type) {
@@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
/* FALL THROUGH */
if (f->op != Audit_not_equal && f->op != Audit_equal)
return -EINVAL;
break;
@@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
@@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 11848df..ce6cbda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
struct inode *inode = d_backing_inode(dentry);
const char *dname = dentry->d_name.name;
struct audit_names *n, *found_parent = NULL, *found_child = NULL;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;
if (!context->in_syscall)
return;
+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE) {
+ if (audit_comparator(parent->i_sb->s_magic,
+ f->op, f->val)) {
+ if (e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
+ }
+ }
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (inode)
handle_one(inode);
--
1.7.1
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Paul Moore]

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().
Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.
Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry. Even if one is found, there may be more than
one mount point. At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.
type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
fix audit_buffer leak and dname error allocation leak audit_log_name
only put audit_name->dentry if it is being replaced
minor cosmetic changes and support fs filter patch
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 19 +++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 31 insertions(+), 5 deletions(-)

...

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 59e60e0..d6e6e4e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -72,6 +72,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
name->gid = inode->i_gid;
name->rdev = inode->i_rdev;
security_inode_getsecid(inode, &name->osid);
+ if (name->dentry) {
+ dput(name->dentry);
+ name->dentry = NULL;
+ }
audit_copy_fcaps(name, dentry);
}
@@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
audit_log_n_untrustedstring(ab, n->name->name,
n->name_len);
}
+ } else if (n->dentry) {
+ char *fullpath;
+ const char *fullpathp = NULL;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (fullpath)
+ fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
+ if (IS_ERR(fullpathp)) {
+ fullpathp = NULL;
+ kfree(fullpath);
+ }
+ audit_log_format(ab, " name=%s(0x%lx):%s",
+ n->dentry->d_sb->s_type->name ?: "?",
+ n->dentry->d_sb->s_magic, fullpathp ?: "?");
} else
audit_log_format(ab, " name=(null)");

While looking this over one more time before merging I realized that
you are missing the curly braces in the "if (fullpath)" if-statement
above. This is an easy fix, and appears to be the Right Thing, so I'm
just going to fix up the patch while merging; take a look at the
result in the audit/next tree and if you have any objections let me
know so I can back it out.

I'm also only merging this patch right now, patch 2/2 needs to wait
until the corresponding userspace is ready so we can test/verify it.

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().
Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.
Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry. Even if one is found, there may be more than
one mount point. At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.
type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
fix audit_buffer leak and dname error allocation leak audit_log_name
only put audit_name->dentry if it is being replaced
minor cosmetic changes and support fs filter patch
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 19 +++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 31 insertions(+), 5 deletions(-)

...

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 59e60e0..d6e6e4e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -72,6 +72,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
name->gid = inode->i_gid;
name->rdev = inode->i_rdev;
security_inode_getsecid(inode, &name->osid);
+ if (name->dentry) {
+ dput(name->dentry);
+ name->dentry = NULL;
+ }
audit_copy_fcaps(name, dentry);
}
@@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
audit_log_n_untrustedstring(ab, n->name->name,
n->name_len);
}
+ } else if (n->dentry) {
+ char *fullpath;
+ const char *fullpathp = NULL;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (fullpath)
+ fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
+ if (IS_ERR(fullpathp)) {
+ fullpathp = NULL;
+ kfree(fullpath);
+ }
+ audit_log_format(ab, " name=%s(0x%lx):%s",
+ n->dentry->d_sb->s_type->name ?: "?",
+ n->dentry->d_sb->s_magic, fullpathp ?: "?");
} else
audit_log_format(ab, " name=(null)");

While looking this over one more time before merging I realized that
you are missing the curly braces in the "if (fullpath)" if-statement
above. This is an easy fix, and appears to be the Right Thing, so I'm
just going to fix up the patch while merging; take a look at the
result in the audit/next tree and if you have any objections let me
know so I can back it out.

Thanks for catching that. That fix looks correct to me.

Post by Paul Moore
I'm also only merging this patch right now, patch 2/2 needs to wait
until the corresponding userspace is ready so we can test/verify it.

I've now got tests to test patch 1 and patch 2 each seperately.

They have to be two seperate manual tests because they each require a
reboot with different rules.

Post by Paul Moore
paul moore

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().
Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.
Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry. Even if one is found, there may be more than
one mount point. At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.
type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
fix audit_buffer leak and dname error allocation leak audit_log_name
only put audit_name->dentry if it is being replaced
minor cosmetic changes and support fs filter patch
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 19 +++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 31 insertions(+), 5 deletions(-)

...

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 59e60e0..d6e6e4e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -72,6 +72,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
name->gid = inode->i_gid;
name->rdev = inode->i_rdev;
security_inode_getsecid(inode, &name->osid);
+ if (name->dentry) {
+ dput(name->dentry);
+ name->dentry = NULL;
+ }
audit_copy_fcaps(name, dentry);
}
@@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
audit_log_n_untrustedstring(ab, n->name->name,
n->name_len);
}
+ } else if (n->dentry) {
+ char *fullpath;
+ const char *fullpathp = NULL;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (fullpath)
+ fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
+ if (IS_ERR(fullpathp)) {
+ fullpathp = NULL;
+ kfree(fullpath);
+ }
+ audit_log_format(ab, " name=%s(0x%lx):%s",
+ n->dentry->d_sb->s_type->name ?: "?",
+ n->dentry->d_sb->s_magic, fullpathp ?: "?");
} else
audit_log_format(ab, " name=(null)");

While looking this over one more time before merging I realized that
you are missing the curly braces in the "if (fullpath)" if-statement
above. This is an easy fix, and appears to be the Right Thing, so I'm
just going to fix up the patch while merging; take a look at the
result in the audit/next tree and if you have any objections let me
know so I can back it out.
I'm also only merging this patch right now, patch 2/2 needs to wait
until the corresponding userspace is ready so we can test/verify it.

The tests have been brought up to date. The first test patch
(syscall_module_path) tests kernel patch 1/2 that adds names for the
NULL paths. The second test patch (syscall_module_path_filter) tests
kernel patch 2/2 and the correpsonding userspace patch to set the
filesystem PATH filters.

See https://github.com/linux-audit/audit-testsuite/pull/42

Post by Paul Moore
paul moore

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Steve Grubb]

Post by Paul Moore

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().

Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field all
by itself called "fstype". You can just leave it as the hex magic number
prepended with 0x and user space can do the lookup from there,

It might be simplest to just apply a corrective patch over top of this one so
that you don't have to muck about with git branches and commit messages.

-Steve

Post by Paul Moore

Post by Richard Guy Briggs
Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.
Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry. Even if one is found, there may be more than
one mount point. At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.
proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
msg=audit(1488317694.446:143): item=797
name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969
dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tracefs_t:s0 nametype=CREATE type=PATH
msg=audit(1488317694.446:143): item=796
name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964
dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tracefs_t:s0 nametype=PARENT ...
type=PATH msg=audit(1488317694.446:143): item=1
name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755
ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0
nametype=CREATE type=PATH msg=audit(1488317694.446:143): item=0
name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0
ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313
success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6
pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod"
subj=system_u:system_r:insmod_t:s0 key="mod-load"
See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
---
fix audit_buffer leak and dname error allocation leak audit_log_name
only put audit_name->dentry if it is being replaced
minor cosmetic changes and support fs filter patch
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 19 +++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 31 insertions(+), 5 deletions(-)

...

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 59e60e0..d6e6e4e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -72,6 +72,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name,
const struct dentry *dentry,>
name->gid = inode->i_gid;
name->rdev = inode->i_rdev;
security_inode_getsecid(inode, &name->osid);
+ if (name->dentry) {
+ dput(name->dentry);
+ name->dentry = NULL;
+ }
audit_copy_fcaps(name, dentry);
}
@@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context,
struct audit_names *n,>
audit_log_n_untrustedstring(ab, n->name->name,
n->name_len);
}
+ } else if (n->dentry) {
+ char *fullpath;
+ const char *fullpathp = NULL;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (fullpath)
+ fullpathp = dentry_path_raw(n->dentry, fullpath,
PATH_MAX); + if (IS_ERR(fullpathp)) {
+ fullpathp = NULL;
+ kfree(fullpath);
+ }
+ audit_log_format(ab, " name=%s(0x%lx):%s",
+ n->dentry->d_sb->s_type->name ?: "?",
"?");>
} else
audit_log_format(ab, " name=(null)");

While looking this over one more time before merging I realized that
you are missing the curly braces in the "if (fullpath)" if-statement
above. This is an easy fix, and appears to be the Right Thing, so I'm
just going to fix up the patch while merging; take a look at the
result in the audit/next tree and if you have any objections let me
know so I can back it out.
I'm also only merging this patch right now, patch 2/2 needs to wait
until the corresponding userspace is ready so we can test/verify it.

[Paul Moore]

Post by Steve Grubb

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().

Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field all
by itself called "fstype". You can just leave it as the hex magic number
prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one so
that you don't have to muck about with git branches and commit messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

[Steve Grubb]

Post by Paul Moore

Post by Steve Grubb

Post by Richard Guy Briggs
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().

Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field
all by itself called "fstype". You can just leave it as the hex magic
number prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one
so that you don't have to muck about with git branches and commit
messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.

Post by Paul Moore
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.

-Steve

Post by Paul Moore
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

[Paul Moore]

...

Post by Steve Grubb

Post by Paul Moore

Post by Steve Grubb
Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field
all by itself called "fstype". You can just leave it as the hex magic
number prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one
so that you don't have to muck about with git branches and commit
messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left. Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.

Post by Steve Grubb

Post by Paul Moore
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.

There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways. Regardless, as you say, the problem can likely be avoided
this time.

Post by Steve Grubb

Post by Paul Moore
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

[Richard Guy Briggs]

Post by Paul Moore
...

Post by Steve Grubb

Post by Paul Moore

Post by Steve Grubb
Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field
all by itself called "fstype". You can just leave it as the hex magic
number prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one
so that you don't have to muck about with git branches and commit
messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left. Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.

I don't really see that this is my choice to include it or not. This is
the upstream maintainer's decision.

I can't say I'd be thrilled to have my name on something that stuffs up
the system though. It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?". How are the analysis tools stymied by a text prefix to a
path that it can't find anyways?

Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

Post by Paul Moore

Post by Steve Grubb

Post by Paul Moore
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.

So why does this make it unsearchable? I still don't understand any
explanations that have been made so far.

Post by Paul Moore
There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways. Regardless, as you say, the problem can likely be avoided
this time.

Post by Steve Grubb

Post by Paul Moore
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

The irony here stuns me. 2/2 was supposed to be the more controvertial
one.

Post by Paul Moore
paul moore

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore
...

Post by Steve Grubb

Post by Paul Moore

Post by Steve Grubb
Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field
all by itself called "fstype". You can just leave it as the hex magic
number prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one
so that you don't have to muck about with git branches and commit
messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left. Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.

I don't really see that this is my choice to include it or not. This is
the upstream maintainer's decision.

You are right, however, while ultimately it isn't your choice I still
wanted to hear your opinion on this as you have put a lot of effort
into this patchset.

Post by Richard Guy Briggs
I can't say I'd be thrilled to have my name on something that stuffs up
the system though. It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.

That confuses me too. My current thinking is that a partial, or
relative, path is not something we want.

Post by Richard Guy Briggs
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?".

Not to mention we still have the relative path problem in this case.

Post by Richard Guy Briggs
How are the analysis tools stymied by a text prefix to a path that it can't find anyways?

I've been wondering the same. My gut feeling isn't a positive comment
so I'll refrain from sharing it here.

Post by Richard Guy Briggs
Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

The odds of agreeing upon a corrective patch and getting it tested and
soaked before the merge window opens is z-e-r-o. As I said earlier,
at the very top of my first response, this isn't an option (I'm hoping
you just missed reading that).

I've been testing audit/next without patch 1/2 this afternoon and it
is still looking okay; unless I see something arguing against it
within the next hour or two that's what I'm going to send up to Linus.

Post by Richard Guy Briggs

Post by Paul Moore

Post by Steve Grubb

Post by Paul Moore
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.

So why does this make it unsearchable? I still don't understand any
explanations that have been made so far.

Agree.

Post by Richard Guy Briggs

Post by Paul Moore
There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways. Regardless, as you say, the problem can likely be avoided
this time.

Post by Steve Grubb

Post by Paul Moore
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

The irony here stuns me. 2/2 was supposed to be the more controvertial
one.

Yes, me too. I never thought patch 1/2 would be the problematic one.
Oh well. Do you have any objection to 2/2 going up to Linus?

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore
...

Post by Steve Grubb

Post by Paul Moore

Post by Steve Grubb
Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field
all by itself called "fstype". You can just leave it as the hex magic
number prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one
so that you don't have to muck about with git branches and commit
messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left. Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.

I don't really see that this is my choice to include it or not. This is
the upstream maintainer's decision.

You are right, however, while ultimately it isn't your choice I still
wanted to hear your opinion on this as you have put a lot of effort
into this patchset.

Post by Richard Guy Briggs
I can't say I'd be thrilled to have my name on something that stuffs up
the system though. It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.

That confuses me too. My current thinking is that a partial, or
relative, path is not something we want.

Post by Richard Guy Briggs
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?".

Not to mention we still have the relative path problem in this case.

Post by Richard Guy Briggs
How are the analysis tools stymied by a text prefix to a path that it can't find anyways?

I've been wondering the same. My gut feeling isn't a positive comment
so I'll refrain from sharing it here.

Post by Richard Guy Briggs
Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

The odds of agreeing upon a corrective patch and getting it tested and
soaked before the merge window opens is z-e-r-o. As I said earlier,
at the very top of my first response, this isn't an option (I'm hoping
you just missed reading that).

Oh, I read that. That's what informed my position. That should help
you make your decision.

Post by Paul Moore
I've been testing audit/next without patch 1/2 this afternoon and it
is still looking okay; unless I see something arguing against it
within the next hour or two that's what I'm going to send up to Linus.

Post by Richard Guy Briggs

Post by Paul Moore

Post by Steve Grubb

Post by Paul Moore
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.

So why does this make it unsearchable? I still don't understand any
explanations that have been made so far.

Agree.

Post by Richard Guy Briggs

Post by Paul Moore
There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways. Regardless, as you say, the problem can likely be avoided
this time.

Post by Steve Grubb

Post by Paul Moore
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

The irony here stuns me. 2/2 was supposed to be the more controvertial
one.

Yes, me too. I never thought patch 1/2 would be the problematic one.
Oh well. Do you have any objection to 2/2 going up to Linus?

They are two fairly different solutions to the same problem. It can
stand on its own.

Post by Paul Moore
paul moore

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Steve Grubb]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Steve Grubb

Post by Paul Moore

Post by Steve Grubb
It might be simplest to just apply a corrective patch over top of
this one so that you don't have to muck about with git branches and
commit messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you
presented its the only one that avoids changing a critical field
format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left.

Its just moving the output of the information a few lines down further in the
code. 10 minutes of work, tops.

Post by Richard Guy Briggs

Post by Paul Moore
Event yanks/reverts carry a risk at this stage, but I consider that the
less risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.

I don't really see that this is my choice to include it or not. This is
the upstream maintainer's decision.
I can't say I'd be thrilled to have my name on something that stuffs up
the system though. It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.

The path should stay. Just the file system type needs decoupling and moving.

Post by Richard Guy Briggs
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?".

I would let it optionally swing in and out at the end of the record. This
should never show up on a normal system because the rules will suppress
generating this information by default. So, it really won't be all that
visible.

Post by Richard Guy Briggs
How are the analysis tools stymied by a text prefix to a path that it can't
find anyways?

Because they do not actually resolve anything in the file system. They take
the event as ground truth and use that. Also, the tools expect name=value.
This has been the way since the beginning. We do not lump multiple independent
values together. And then what if the path has a special character in it? The
whole value then has to be encoded. And I don't think the patch is using
untrusted string like it should.

Post by Richard Guy Briggs
Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

Post by Paul Moore

Post by Steve Grubb

Post by Paul Moore
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field
that has been defined since 2005. The "name" field is very important.
It's used in quite a few reports, its used in the text format, it's
searchable, and we have a dictionary that defines exactly what it is.
Fields that are searchable and used in common reports cannot be changed
without a whole lot of coordination. I'm also disappointed to have to
point out that new information should go in its own field. I thought
this was common knowledge. In any event, it was caught and problems can
be avoided.

So why does this make it unsearchable?

I didn't say it makes in unsearchable, but now that you mention it...it does
in one case. Searchable fields are more important. They typically are the
object or subject or some kind of special attribute that is commonly searched
on to group events. Searchable fields can either be partial or full word
match. By combining information in the same field, it will change this
behavior. The path name is the object of the event. By combining information
that is not the object with it, everyone will have to change and update their
software to handle this change in behavior.

Post by Richard Guy Briggs
I still don't understand any explanations that have been made so far

Try ausearch --text on those events, or aureport --file.

-Steve

[Paul Moore]

Post by Steve Grubb

Post by Richard Guy Briggs

Post by Paul Moore

Post by Steve Grubb

Post by Paul Moore

Post by Steve Grubb
It might be simplest to just apply a corrective patch over top of
this one so that you don't have to muck about with git branches and
commit messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you
presented its the only one that avoids changing a critical field
format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left.

Its just moving the output of the information a few lines down further in the
code. 10 minutes of work, tops.

It's like you don't even bother reading why I write ... it's not about
the amount of time needed to make the change, it's the other stuff I
mentioned. Regardless, it's a moot point now, the patch is out and it
isn't going up in the currently open merge window.

Post by Steve Grubb

Post by Richard Guy Briggs

Post by Paul Moore
Event yanks/reverts carry a risk at this stage, but I consider that the
less risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.

I don't really see that this is my choice to include it or not. This is
the upstream maintainer's decision.
I can't say I'd be thrilled to have my name on something that stuffs up
the system though. It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.

The path should stay. Just the file system type needs decoupling and moving.

See my previous comments about relative pathnames, specifially the
part about me not being a fan of them in the PATH records.