* [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
@ 2017-04-04 9:21 Richard Guy Briggs
2017-04-04 9:21 ` [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-04-04 9:21 UTC (permalink / raw)
To: linux-kernel, linux-audit
Cc: Richard Guy Briggs, Steven Rostedt, Ingo Molnar,
Greg Kroah-Hartman, Al Viro, Eric Paris, Paul Moore, Steve Grubb
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 <rgb@redhat.com>
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 16 ++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index aba3a26..367a03a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -241,7 +241,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);
@@ -306,7 +306,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);
@@ -487,7 +487,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,
@@ -501,7 +501,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 25dd70a..7d83c5a 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -66,6 +66,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -1884,6 +1885,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);
}
@@ -1925,6 +1930,17 @@ 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;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (!fullpath)
+ return;
+ fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
+ 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 144b7eb..2a11583 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -84,6 +84,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 4db32e8..b3797c7 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);
}
@@ -1858,7 +1861,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;
@@ -1914,6 +1917,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) {
@@ -1935,6 +1939,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);
--
1.7.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
2017-04-04 9:21 [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
@ 2017-04-04 9:21 ` Richard Guy Briggs
2017-04-04 10:02 ` Richard Guy Briggs
2017-05-30 21:30 ` Paul Moore
2017-04-04 21:19 ` [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
2017-05-30 21:21 ` Paul Moore
2 siblings, 2 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-04-04 9:21 UTC (permalink / raw)
To: linux-kernel, linux-audit
Cc: Richard Guy Briggs, Steven Rostedt, Ingo Molnar,
Greg Kroah-Hartman, Al Viro, Eric Paris, Paul Moore, Steve Grubb
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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
-a never,path -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
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
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 3c02bb2..0464910 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_PATH 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). */
@@ -334,12 +336,14 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_PATH 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_SESSIONID_FILTER | \
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_PATH)
/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 239d11c..3e0ccf2 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 @@ static struct list_head audit_rules_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 inline struct audit_entry *audit_to_entry_common(struct audit_rule_data *
#endif
case AUDIT_FILTER_USER:
case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_PATH:
;
}
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_PATH)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_PATH:
+ 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_PATH:
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_PATH:
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b3797c7..a12531f 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1868,10 +1868,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_PATH];
+ 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
2017-04-04 9:21 ` [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
@ 2017-04-04 10:02 ` Richard Guy Briggs
2017-05-30 21:30 ` Paul Moore
1 sibling, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-04-04 10:02 UTC (permalink / raw)
To: linux-kernel, linux-audit
Cc: Steven Rostedt, Ingo Molnar, Greg Kroah-Hartman, Al Viro,
Eric Paris, Paul Moore, Steve Grubb
On 2017-04-04 05:21, Richard Guy Briggs wrote:
> 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
> -a never,path -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
Sorry, this issue number is wrong.
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 <rgb@redhat.com>
> ---
> 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 3c02bb2..0464910 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_PATH 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). */
> @@ -334,12 +336,14 @@ enum {
> #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> +#define AUDIT_FEATURE_BITMAP_FILTER_PATH 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_SESSIONID_FILTER | \
> - AUDIT_FEATURE_BITMAP_LOST_RESET)
> + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> + AUDIT_FEATURE_BITMAP_FILTER_PATH)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 239d11c..3e0ccf2 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 @@ static struct list_head audit_rules_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 inline struct audit_entry *audit_to_entry_common(struct audit_rule_data *
> #endif
> case AUDIT_FILTER_USER:
> case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_PATH:
> ;
> }
> 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_PATH)
> + return -EINVAL;
> + break;
> + }
> +
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_PATH:
> + 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_PATH:
> 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_PATH:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b3797c7..a12531f 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1868,10 +1868,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_PATH];
> + 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
>
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
@ 2017-04-04 10:02 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-04-04 10:02 UTC (permalink / raw)
To: linux-kernel, linux-audit
Cc: Greg Kroah-Hartman, Ingo Molnar, Steven Rostedt, Al Viro
On 2017-04-04 05:21, Richard Guy Briggs wrote:
> 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
> -a never,path -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
Sorry, this issue number is wrong.
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 <rgb@redhat.com>
> ---
> 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 3c02bb2..0464910 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_PATH 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). */
> @@ -334,12 +336,14 @@ enum {
> #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> +#define AUDIT_FEATURE_BITMAP_FILTER_PATH 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_SESSIONID_FILTER | \
> - AUDIT_FEATURE_BITMAP_LOST_RESET)
> + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> + AUDIT_FEATURE_BITMAP_FILTER_PATH)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 239d11c..3e0ccf2 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 @@ static struct list_head audit_rules_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 inline struct audit_entry *audit_to_entry_common(struct audit_rule_data *
> #endif
> case AUDIT_FILTER_USER:
> case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_PATH:
> ;
> }
> 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_PATH)
> + return -EINVAL;
> + break;
> + }
> +
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_PATH:
> + 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_PATH:
> 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_PATH:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b3797c7..a12531f 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1868,10 +1868,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_PATH];
> + 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
>
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-04-04 9:21 [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
2017-04-04 9:21 ` [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
@ 2017-04-04 21:19 ` Paul Moore
2017-04-05 4:18 ` Richard Guy Briggs
2017-05-30 21:21 ` Paul Moore
2 siblings, 1 reply; 22+ messages in thread
From: Paul Moore @ 2017-04-04 21:19 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> 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 <rgb@redhat.com>
> ---
> include/linux/audit.h | 8 ++++----
> kernel/audit.c | 16 ++++++++++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 8 +++++++-
> 4 files changed, 28 insertions(+), 5 deletions(-)
I see the "ALT4" in the subject line which gives me pause; does this
mean you plan further development for the other potential fixes, or
did you just include it on this patch for better tracking with it's
predecessor?
I'm basically asking what to expect. Should I review this for
inclusion, or wait for other alternatives?
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index aba3a26..367a03a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -241,7 +241,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);
> @@ -306,7 +306,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);
> @@ -487,7 +487,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,
> @@ -501,7 +501,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 25dd70a..7d83c5a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -66,6 +66,7 @@
> #include <linux/freezer.h>
> #include <linux/pid_namespace.h>
> #include <net/netns/generic.h>
> +#include <linux/dcache.h>
>
> #include "audit.h"
>
> @@ -1884,6 +1885,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);
> }
>
> @@ -1925,6 +1930,17 @@ 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;
> +
> + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> + if (!fullpath)
> + return;
> + fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> + 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 144b7eb..2a11583 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -84,6 +84,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 4db32e8..b3797c7 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);
> }
> @@ -1858,7 +1861,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;
> @@ -1914,6 +1917,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) {
> @@ -1935,6 +1939,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);
>
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-04-04 21:19 ` [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
@ 2017-04-05 4:18 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-04-05 4:18 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On 2017-04-04 17:19, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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 <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 16 ++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 28 insertions(+), 5 deletions(-)
>
> I see the "ALT4" in the subject line which gives me pause; does this
> mean you plan further development for the other potential fixes, or
> did you just include it on this patch for better tracking with it's
> predecessor?
The "ALT4" was included for tracking. This is the planned path forward,
abandonning the rest of the "ALT*"s.
> I'm basically asking what to expect. Should I review this for
> inclusion, or wait for other alternatives?
Please review.
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index aba3a26..367a03a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -241,7 +241,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);
> > @@ -306,7 +306,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);
> > @@ -487,7 +487,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,
> > @@ -501,7 +501,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 25dd70a..7d83c5a 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -66,6 +66,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -1884,6 +1885,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);
> > }
> >
> > @@ -1925,6 +1930,17 @@ 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;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (!fullpath)
> > + return;
> > + fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> > + 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 144b7eb..2a11583 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -84,6 +84,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 4db32e8..b3797c7 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);
> > }
> > @@ -1858,7 +1861,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;
> > @@ -1914,6 +1917,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) {
> > @@ -1935,6 +1939,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);
> >
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
@ 2017-04-05 4:18 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-04-05 4:18 UTC (permalink / raw)
To: Paul Moore
Cc: Greg Kroah-Hartman, linux-kernel, Steven Rostedt, linux-audit,
Al Viro, Ingo Molnar
On 2017-04-04 17:19, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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 <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 16 ++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 28 insertions(+), 5 deletions(-)
>
> I see the "ALT4" in the subject line which gives me pause; does this
> mean you plan further development for the other potential fixes, or
> did you just include it on this patch for better tracking with it's
> predecessor?
The "ALT4" was included for tracking. This is the planned path forward,
abandonning the rest of the "ALT*"s.
> I'm basically asking what to expect. Should I review this for
> inclusion, or wait for other alternatives?
Please review.
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index aba3a26..367a03a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -241,7 +241,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);
> > @@ -306,7 +306,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);
> > @@ -487,7 +487,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,
> > @@ -501,7 +501,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 25dd70a..7d83c5a 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -66,6 +66,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -1884,6 +1885,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);
> > }
> >
> > @@ -1925,6 +1930,17 @@ 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;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (!fullpath)
> > + return;
> > + fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> > + 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 144b7eb..2a11583 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -84,6 +84,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 4db32e8..b3797c7 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);
> > }
> > @@ -1858,7 +1861,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;
> > @@ -1914,6 +1917,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) {
> > @@ -1935,6 +1939,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);
> >
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-04-04 9:21 [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
2017-04-04 9:21 ` [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
2017-04-04 21:19 ` [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
@ 2017-05-30 21:21 ` Paul Moore
2017-06-27 21:11 ` Richard Guy Briggs
2 siblings, 1 reply; 22+ messages in thread
From: Paul Moore @ 2017-05-30 21:21 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> 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 <rgb@redhat.com>
> ---
> include/linux/audit.h | 8 ++++----
> kernel/audit.c | 16 ++++++++++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 8 +++++++-
> 4 files changed, 28 insertions(+), 5 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 25dd70a..7d83c5a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -66,6 +66,7 @@
> #include <linux/freezer.h>
> #include <linux/pid_namespace.h>
> #include <net/netns/generic.h>
> +#include <linux/dcache.h>
>
> #include "audit.h"
>
> @@ -1884,6 +1885,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;
> + }
Out of curiosity, what terrible things happen if we take a reference
to a non-NULL dentry passed to audit_copy_inode() and store it in
name->dentry? Does performance tank?
Also out of curiosity, why do we want to drop a dentry reference here
if one already exists?
> audit_copy_fcaps(name, dentry);
> }
>
> @@ -1925,6 +1930,17 @@ 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;
> +
> + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> + if (!fullpath)
> + return;
I'm wondering if there is some value in still emitting the record if
the kmalloc() fails, just with the name field set as the unset "?"
value, e.g. "name=?". Thoughts?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
2017-04-04 9:21 ` [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
2017-04-04 10:02 ` Richard Guy Briggs
@ 2017-05-30 21:30 ` Paul Moore
2017-06-27 20:45 ` Richard Guy Briggs
1 sibling, 1 reply; 22+ messages in thread
From: Paul Moore @ 2017-05-30 21:30 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
> -a never,path -F fstype=0x64626720 -F key=ignore_debugfs
Trying to look into the future I wonder if we are ever going to need
to expand the "path" filtering to regular inode lookups, e.g.
audit_inode()?
> Arguably the better way to address this issue is to disable tracefs and
> debugfs on boot from production systems.
Agreed. A good goal whenever possible, but that is a larger topic
beyond the scope of this patchset.
> See: https://github.com/linux-audit/audit-kernel/issues/16
> Test case: https://github.com/linux-audit/audit-testsuite/issues/42
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> 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 3c02bb2..0464910 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_PATH 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). */
> @@ -334,12 +336,14 @@ enum {
> #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> +#define AUDIT_FEATURE_BITMAP_FILTER_PATH 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_SESSIONID_FILTER | \
> - AUDIT_FEATURE_BITMAP_LOST_RESET)
> + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> + AUDIT_FEATURE_BITMAP_FILTER_PATH)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 239d11c..3e0ccf2 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 @@ static struct list_head audit_rules_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 inline struct audit_entry *audit_to_entry_common(struct audit_rule_data *
> #endif
> case AUDIT_FILTER_USER:
> case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_PATH:
> ;
> }
> 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_PATH)
> + return -EINVAL;
> + break;
> + }
> +
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_PATH:
> + 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_PATH:
> 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_PATH:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b3797c7..a12531f 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1868,10 +1868,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_PATH];
> + 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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
2017-05-30 21:30 ` Paul Moore
@ 2017-06-27 20:45 ` Richard Guy Briggs
2017-06-28 19:08 ` Paul Moore
0 siblings, 1 reply; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-27 20:45 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On 2017-05-30 17:30, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
> > -a never,path -F fstype=0x64626720 -F key=ignore_debugfs
>
> Trying to look into the future I wonder if we are ever going to need
> to expand the "path" filtering to regular inode lookups, e.g.
> audit_inode()?
That thought had occurred to me. Do you see any concern with that that
would affect this patch in terms of naming?
>
I could see expanding this filter to include other filter fields though
nothing specific comes to mind now.
> > Arguably the better way to address this issue is to disable tracefs and
> > debugfs on boot from production systems.
>
> Agreed. A good goal whenever possible, but that is a larger topic
> beyond the scope of this patchset.
>
> > See: https://github.com/linux-audit/audit-kernel/issues/16
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > 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 3c02bb2..0464910 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_PATH 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). */
> > @@ -334,12 +336,14 @@ enum {
> > #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004
> > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> > +#define AUDIT_FEATURE_BITMAP_FILTER_PATH 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_SESSIONID_FILTER | \
> > - AUDIT_FEATURE_BITMAP_LOST_RESET)
> > + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > + AUDIT_FEATURE_BITMAP_FILTER_PATH)
> >
> > /* deprecated: AUDIT_VERSION_* */
> > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 239d11c..3e0ccf2 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 @@ static struct list_head audit_rules_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 inline struct audit_entry *audit_to_entry_common(struct audit_rule_data *
> > #endif
> > case AUDIT_FILTER_USER:
> > case AUDIT_FILTER_TYPE:
> > + case AUDIT_FILTER_PATH:
> > ;
> > }
> > 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_PATH)
> > + return -EINVAL;
> > + break;
> > + }
> > +
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_PATH:
> > + 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_PATH:
> > 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_PATH:
> > dont_count = 1;
> > + }
> > #endif
> >
> > mutex_lock(&audit_filter_mutex);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index b3797c7..a12531f 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1868,10 +1868,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_PATH];
> > + 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
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-05-30 21:21 ` Paul Moore
@ 2017-06-27 21:11 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-27 21:11 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On 2017-05-30 17:21, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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 <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 16 ++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 28 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 25dd70a..7d83c5a 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -66,6 +66,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -1884,6 +1885,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;
> > + }
>
> Out of curiosity, what terrible things happen if we take a reference
> to a non-NULL dentry passed to audit_copy_inode() and store it in
> name->dentry? Does performance tank?
Interesting idea. Right now it is optimized to only take a reference to
the dentry's parent dentry in the case we're handed an anonymous entry.
Most of the time it will never be used even though we invest in the
overhead of taking a reference count. Besides, __audit_inode_child()
hands in a NULL for the dentry parameter to audit_copy_inode(). I'm
assuming you are hinting at also using that dentry to compare the
audit_names entry, which I think it a bad idea since there could be
multiple paths to access a dentry. I did orignially have another patch
that would have tried to use that as well, which didn't seem to hurt,
but I didn't think was worth upstreaming.
> Also out of curiosity, why do we want to drop a dentry reference here
> if one already exists?
I think we want to drop a dentry reference here because this inode child
could be a subsequent access to the same dentry with a full path,
removing the need to cache this dentry information in the first place.
> > audit_copy_fcaps(name, dentry);
> > }
> >
> > @@ -1925,6 +1930,17 @@ 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;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (!fullpath)
> > + return;
>
> I'm wondering if there is some value in still emitting the record if
> the kmalloc() fails, just with the name field set as the unset "?"
> value, e.g. "name=?". Thoughts?
Possibly. We've got much bigger problems if that happens, but this
sounds like a good defensive coding approach. I'm even tempted to call
audit_panic().
> paul moore
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
@ 2017-06-27 21:11 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-27 21:11 UTC (permalink / raw)
To: Paul Moore
Cc: Greg Kroah-Hartman, linux-kernel, Steven Rostedt, linux-audit,
Al Viro, Ingo Molnar
On 2017-05-30 17:21, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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 <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 16 ++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 28 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 25dd70a..7d83c5a 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -66,6 +66,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -1884,6 +1885,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;
> > + }
>
> Out of curiosity, what terrible things happen if we take a reference
> to a non-NULL dentry passed to audit_copy_inode() and store it in
> name->dentry? Does performance tank?
Interesting idea. Right now it is optimized to only take a reference to
the dentry's parent dentry in the case we're handed an anonymous entry.
Most of the time it will never be used even though we invest in the
overhead of taking a reference count. Besides, __audit_inode_child()
hands in a NULL for the dentry parameter to audit_copy_inode(). I'm
assuming you are hinting at also using that dentry to compare the
audit_names entry, which I think it a bad idea since there could be
multiple paths to access a dentry. I did orignially have another patch
that would have tried to use that as well, which didn't seem to hurt,
but I didn't think was worth upstreaming.
> Also out of curiosity, why do we want to drop a dentry reference here
> if one already exists?
I think we want to drop a dentry reference here because this inode child
could be a subsequent access to the same dentry with a full path,
removing the need to cache this dentry information in the first place.
> > audit_copy_fcaps(name, dentry);
> > }
> >
> > @@ -1925,6 +1930,17 @@ 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;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (!fullpath)
> > + return;
>
> I'm wondering if there is some value in still emitting the record if
> the kmalloc() fails, just with the name field set as the unset "?"
> value, e.g. "name=?". Thoughts?
Possibly. We've got much bigger problems if that happens, but this
sounds like a good defensive coding approach. I'm even tempted to call
audit_panic().
> paul moore
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-06-27 21:11 ` Richard Guy Briggs
(?)
@ 2017-06-28 19:03 ` Paul Moore
2017-06-29 21:21 ` Richard Guy Briggs
2017-08-11 6:36 ` Richard Guy Briggs
-1 siblings, 2 replies; 22+ messages in thread
From: Paul Moore @ 2017-06-28 19:03 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On Tue, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-05-30 17:21, Paul Moore wrote:
>> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
...
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 25dd70a..7d83c5a 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -66,6 +66,7 @@
>> > #include <linux/freezer.h>
>> > #include <linux/pid_namespace.h>
>> > #include <net/netns/generic.h>
>> > +#include <linux/dcache.h>
>> >
>> > #include "audit.h"
>> >
>> > @@ -1884,6 +1885,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;
>> > + }
>>
>> Out of curiosity, what terrible things happen if we take a reference
>> to a non-NULL dentry passed to audit_copy_inode() and store it in
>> name->dentry? Does performance tank?
>
> Interesting idea. Right now it is optimized to only take a reference to
> the dentry's parent dentry in the case we're handed an anonymous entry.
> Most of the time it will never be used even though we invest in the
> overhead of taking a reference count. Besides, __audit_inode_child()
> hands in a NULL for the dentry parameter to audit_copy_inode().
[NOTE: audit_copy_inode() hands a NULL dentry only in the anonymous parent case]
I believe I was just thinking of less conditional handling, especially
when reference counts are concerned. I'm just trying to limit future
headaches, but I suspect the perf cost would be problematic, and as
you point out, there is no *need* for the majority of cases.
Looking at this again today, why would we want to clear name->dentry
in audit_copy_inode() if it is already set? Does that ever happen?
I'm not sure it does ...
> I'm
> assuming you are hinting at also using that dentry to compare the
> audit_names entry, which I think it a bad idea since there could be
> multiple paths to access a dentry. I did orignially have another patch
> that would have tried to use that as well, which didn't seem to hurt,
> but I didn't think was worth upstreaming.
No, I wasn't thinking that, the dev/inode numbers should be sufficient
in those cases I believe; I'm not sure the dentry would help us here.
>> Also out of curiosity, why do we want to drop a dentry reference here
>> if one already exists?
>
> I think we want to drop a dentry reference here because this inode child
> could be a subsequent access to the same dentry with a full path,
> removing the need to cache this dentry information in the first place.
Related to my comment above from today ... what code path please?
>> > @@ -1925,6 +1930,17 @@ 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;
>> > +
>> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
>> > + if (!fullpath)
>> > + return;
>>
>> I'm wondering if there is some value in still emitting the record if
>> the kmalloc() fails, just with the name field set as the unset "?"
>> value, e.g. "name=?". Thoughts?
>
> Possibly. We've got much bigger problems if that happens, but this
> sounds like a good defensive coding approach. I'm even tempted to call
> audit_panic().
No audit_panic(). We've still got good information that we can
record, e.g. dev/inode numbers; let's just print "name=?" and go on
our way recording the rest of the information. This is in keeping
with the current audit_log_name() error handling.
At the very least you need to clean up here instead of just returning.
As the patch currently stands I believe this will end up leaking an
audit_buffer.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
2017-06-27 20:45 ` Richard Guy Briggs
@ 2017-06-28 19:08 ` Paul Moore
2017-06-28 21:25 ` Richard Guy Briggs
0 siblings, 1 reply; 22+ messages in thread
From: Paul Moore @ 2017-06-28 19:08 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On Tue, Jun 27, 2017 at 4:45 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-05-30 17:30, Paul Moore wrote:
>> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
>> > -a never,path -F fstype=0x64626720 -F key=ignore_debugfs
>>
>> Trying to look into the future I wonder if we are ever going to need
>> to expand the "path" filtering to regular inode lookups, e.g.
>> audit_inode()?
>
> That thought had occurred to me. Do you see any concern with that that
> would affect this patch in terms of naming?
Well, you want to change this to "fs" now instead of "path", right? I
think that removes my concerns.
> I could see expanding this filter to include other filter fields though
> nothing specific comes to mind now.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
2017-06-28 19:08 ` Paul Moore
@ 2017-06-28 21:25 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-28 21:25 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On 2017-06-28 15:08, Paul Moore wrote:
> On Tue, Jun 27, 2017 at 4:45 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-05-30 17:30, Paul Moore wrote:
> >> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
> >> > -a never,path -F fstype=0x64626720 -F key=ignore_debugfs
> >>
> >> Trying to look into the future I wonder if we are ever going to need
> >> to expand the "path" filtering to regular inode lookups, e.g.
> >> audit_inode()?
> >
> > That thought had occurred to me. Do you see any concern with that that
> > would affect this patch in terms of naming?
>
> Well, you want to change this to "fs" now instead of "path", right? I
> think that removes my concerns.
That's fixed in my cleanup that I'm waiting to push. In fact it is
"filesystem" in the userspace patch.
> > I could see expanding this filter to include other filter fields though
> > nothing specific comes to mind now.
>
> paul moore
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic
@ 2017-06-28 21:25 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-28 21:25 UTC (permalink / raw)
To: Paul Moore
Cc: Greg Kroah-Hartman, linux-kernel, Steven Rostedt, linux-audit,
Al Viro, Ingo Molnar
On 2017-06-28 15:08, Paul Moore wrote:
> On Tue, Jun 27, 2017 at 4:45 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-05-30 17:30, Paul Moore wrote:
> >> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > 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_PATH", 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,path -F fstype=0x74726163 -F key=ignore_tracefs
> >> > -a never,path -F fstype=0x64626720 -F key=ignore_debugfs
> >>
> >> Trying to look into the future I wonder if we are ever going to need
> >> to expand the "path" filtering to regular inode lookups, e.g.
> >> audit_inode()?
> >
> > That thought had occurred to me. Do you see any concern with that that
> > would affect this patch in terms of naming?
>
> Well, you want to change this to "fs" now instead of "path", right? I
> think that removes my concerns.
That's fixed in my cleanup that I'm waiting to push. In fact it is
"filesystem" in the userspace patch.
> > I could see expanding this filter to include other filter fields though
> > nothing specific comes to mind now.
>
> paul moore
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-06-28 19:03 ` Paul Moore
@ 2017-06-29 21:21 ` Richard Guy Briggs
2017-06-29 23:58 ` Steven Rostedt
2017-08-11 6:36 ` Richard Guy Briggs
1 sibling, 1 reply; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-29 21:21 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, linux-audit, Greg Kroah-Hartman, Steven Rostedt,
Ingo Molnar, Al Viro
On 2017-06-28 15:03, Paul Moore wrote:
> On Tue, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-05-30 17:21, Paul Moore wrote:
> >> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>
> ...
>
> >> > diff --git a/kernel/audit.c b/kernel/audit.c
> >> > index 25dd70a..7d83c5a 100644
> >> > --- a/kernel/audit.c
> >> > +++ b/kernel/audit.c
> >> > @@ -66,6 +66,7 @@
> >> > #include <linux/freezer.h>
> >> > #include <linux/pid_namespace.h>
> >> > #include <net/netns/generic.h>
> >> > +#include <linux/dcache.h>
> >> >
> >> > #include "audit.h"
> >> >
> >> > @@ -1884,6 +1885,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;
> >> > + }
> >>
> >> Out of curiosity, what terrible things happen if we take a reference
> >> to a non-NULL dentry passed to audit_copy_inode() and store it in
> >> name->dentry? Does performance tank?
> >
> > Interesting idea. Right now it is optimized to only take a reference to
> > the dentry's parent dentry in the case we're handed an anonymous entry.
> > Most of the time it will never be used even though we invest in the
> > overhead of taking a reference count. Besides, __audit_inode_child()
> > hands in a NULL for the dentry parameter to audit_copy_inode().
>
> [NOTE: audit_copy_inode() hands a NULL dentry only in the anonymous parent case]
>
> I believe I was just thinking of less conditional handling, especially
> when reference counts are concerned. I'm just trying to limit future
> headaches, but I suspect the perf cost would be problematic, and as
> you point out, there is no *need* for the majority of cases.
I don't like the conditional either, but it appears to be so seldom
needed that it looks like the lesser of evils.
> Looking at this again today, why would we want to clear name->dentry
> in audit_copy_inode() if it is already set? Does that ever happen?
> I'm not sure it does ...
It has been nearly 3 months since I coded that, so I'll have to dive in
and re-analyse what I was thinking at that time. I think that rationale
was that if audit_copy_inode() is called again on that audit_name struct
that it could be called by audit_log_link_denied() or __audit_inode()
not needing the dentry reference or even by __audit_inode_child() and
have it replaced, needing a reference count correction.
So I'm gathering that you think we should just leave it there unless
that reference gets updated/replaced which would only happen in the case
of the child of an anonymous parent. If the other methods succeed, that
reference will simply be ignored and freed when the context does.
> > I'm
> > assuming you are hinting at also using that dentry to compare the
> > audit_names entry, which I think it a bad idea since there could be
> > multiple paths to access a dentry. I did orignially have another patch
> > that would have tried to use that as well, which didn't seem to hurt,
> > but I didn't think was worth upstreaming.
>
> No, I wasn't thinking that, the dev/inode numbers should be sufficient
> in those cases I believe; I'm not sure the dentry would help us here.
>
> >> Also out of curiosity, why do we want to drop a dentry reference here
> >> if one already exists?
> >
> > I think we want to drop a dentry reference here because this inode child
> > could be a subsequent access to the same dentry with a full path,
> > removing the need to cache this dentry information in the first place.
>
> Related to my comment above from today ... what code path please?
First reference to an anonymous parent, then a subsequent call
referencing its child. Since the anonymous parent and its child don't
have a name stored, it can never match a subsequent parent lookup.
The more I look at this, I think I missed another bug elsewhere failing
to call audit_getname() from getname_flags() or getname_kernel() that is
causing this anonymous parent.
> >> > @@ -1925,6 +1930,17 @@ 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;
> >> > +
> >> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> >> > + if (!fullpath)
> >> > + return;
> >>
> >> I'm wondering if there is some value in still emitting the record if
> >> the kmalloc() fails, just with the name field set as the unset "?"
> >> value, e.g. "name=?". Thoughts?
> >
> > Possibly. We've got much bigger problems if that happens, but this
> > sounds like a good defensive coding approach. I'm even tempted to call
> > audit_panic().
>
> No audit_panic(). We've still got good information that we can
> record, e.g. dev/inode numbers; let's just print "name=?" and go on
> our way recording the rest of the information. This is in keeping
> with the current audit_log_name() error handling.
Would you be ok with something in-line like "name=(error)" to indicate
the information is there but an outside influence prevented it from
being printed? (The "?" was my addition.)
There *is* precedent and an existing method in audit_log_name() to panic.
Now that I look at the audit_log_name() patch again, I see a bug in
checking the result of dentry_path_raw().
> At the very least you need to clean up here instead of just returning.
> As the patch currently stands I believe this will end up leaking an
> audit_buffer.
I now see there is a buffer leak that needs to be addressed too.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-06-29 21:21 ` Richard Guy Briggs
@ 2017-06-29 23:58 ` Steven Rostedt
2017-06-30 13:07 ` Richard Guy Briggs
0 siblings, 1 reply; 22+ messages in thread
From: Steven Rostedt @ 2017-06-29 23:58 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, linux-kernel, linux-audit, Greg Kroah-Hartman,
Ingo Molnar, Al Viro
On Thu, 29 Jun 2017 17:21:22 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:
> > Looking at this again today, why would we want to clear name->dentry
> > in audit_copy_inode() if it is already set? Does that ever happen?
> > I'm not sure it does ...
>
> It has been nearly 3 months since I coded that, so I'll have to dive in
> and re-analyse what I was thinking at that time. I think that rationale
> was that if audit_copy_inode() is called again on that audit_name struct
> that it could be called by audit_log_link_denied() or __audit_inode()
> not needing the dentry reference or even by __audit_inode_child() and
> have it replaced, needing a reference count correction.
>
Just a note. If after 3 months you need to re-analyze, you either need
to design things simpler, or have better comments in the code.
-- Steve
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-06-29 23:58 ` Steven Rostedt
@ 2017-06-30 13:07 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-30 13:07 UTC (permalink / raw)
To: Steven Rostedt
Cc: Paul Moore, linux-kernel, linux-audit, Greg Kroah-Hartman,
Ingo Molnar, Al Viro
On 2017-06-29 19:58, Steven Rostedt wrote:
> On Thu, 29 Jun 2017 17:21:22 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
>
> > > Looking at this again today, why would we want to clear name->dentry
> > > in audit_copy_inode() if it is already set? Does that ever happen?
> > > I'm not sure it does ...
> >
> > It has been nearly 3 months since I coded that, so I'll have to dive in
> > and re-analyse what I was thinking at that time. I think that rationale
> > was that if audit_copy_inode() is called again on that audit_name struct
> > that it could be called by audit_log_link_denied() or __audit_inode()
> > not needing the dentry reference or even by __audit_inode_child() and
> > have it replaced, needing a reference count correction.
>
> Just a note. If after 3 months you need to re-analyze, you either need
> to design things simpler, or have better comments in the code.
Yep, both occurred to me. ;-) Thanks Steve.
> -- Steve
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
@ 2017-06-30 13:07 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-06-30 13:07 UTC (permalink / raw)
To: Steven Rostedt
Cc: Greg Kroah-Hartman, linux-kernel, linux-audit, Al Viro, Ingo Molnar
On 2017-06-29 19:58, Steven Rostedt wrote:
> On Thu, 29 Jun 2017 17:21:22 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
>
> > > Looking at this again today, why would we want to clear name->dentry
> > > in audit_copy_inode() if it is already set? Does that ever happen?
> > > I'm not sure it does ...
> >
> > It has been nearly 3 months since I coded that, so I'll have to dive in
> > and re-analyse what I was thinking at that time. I think that rationale
> > was that if audit_copy_inode() is called again on that audit_name struct
> > that it could be called by audit_log_link_denied() or __audit_inode()
> > not needing the dentry reference or even by __audit_inode_child() and
> > have it replaced, needing a reference count correction.
>
> Just a note. If after 3 months you need to re-analyze, you either need
> to design things simpler, or have better comments in the code.
Yep, both occurred to me. ;-) Thanks Steve.
> -- Steve
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-06-28 19:03 ` Paul Moore
2017-06-29 21:21 ` Richard Guy Briggs
@ 2017-08-11 6:36 ` Richard Guy Briggs
2017-08-14 4:32 ` Richard Guy Briggs
1 sibling, 1 reply; 22+ messages in thread
From: Richard Guy Briggs @ 2017-08-11 6:36 UTC (permalink / raw)
To: Paul Moore
Cc: Greg Kroah-Hartman, linux-kernel, Steven Rostedt, linux-audit,
Al Viro, Ingo Molnar
On 2017-06-28 15:03, Paul Moore wrote:
> On Tue, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-05-30 17:21, Paul Moore wrote:
> >> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>
> ...
>
> >> > diff --git a/kernel/audit.c b/kernel/audit.c
> >> > index 25dd70a..7d83c5a 100644
> >> > --- a/kernel/audit.c
> >> > +++ b/kernel/audit.c
> >> > @@ -66,6 +66,7 @@
> >> > #include <linux/freezer.h>
> >> > #include <linux/pid_namespace.h>
> >> > #include <net/netns/generic.h>
> >> > +#include <linux/dcache.h>
> >> >
> >> > #include "audit.h"
> >> >
> >> > @@ -1884,6 +1885,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;
> >> > + }
> >>
> >> Out of curiosity, what terrible things happen if we take a reference
> >> to a non-NULL dentry passed to audit_copy_inode() and store it in
> >> name->dentry? Does performance tank?
> >
> > Interesting idea. Right now it is optimized to only take a reference to
> > the dentry's parent dentry in the case we're handed an anonymous entry.
> > Most of the time it will never be used even though we invest in the
> > overhead of taking a reference count. Besides, __audit_inode_child()
> > hands in a NULL for the dentry parameter to audit_copy_inode().
>
> [NOTE: audit_copy_inode() hands a NULL dentry only in the anonymous parent case]
>
> I believe I was just thinking of less conditional handling, especially
> when reference counts are concerned. I'm just trying to limit future
> headaches, but I suspect the perf cost would be problematic, and as
> you point out, there is no *need* for the majority of cases.
>
> Looking at this again today, why would we want to clear name->dentry
> in audit_copy_inode() if it is already set? Does that ever happen?
> I'm not sure it does ...
Ok, I just tried re-writing that part to dput that dentry only for the
child of an anonymous parent rather than whenever audit_copy_inode() was
called and I ended up with a:
BUG: Dentry ffff8800338f1dc0{i=369a,n=stderr} still in use (1) [unmount of tmpfs tmpfs]
WARNING: CPU: 0 PID: 387 at fs/dcache.c:1445 umount_check+0x99/0xc0
This was after rebasing on a recent audit-next based on 4.11 rather than
the previous based on a 4.8 audit-next. Something else changed in the
kernel or kernel config between these two points. I was getting some
"INFO: suspicious RCU usage." warnings in idmap for NFS on the earlier
kernel and that is no longer happenning, and I'm no longer getting any
tracefs audit PATH entries in the more recent kernel which suggests that
whatever was causing the anonymous parent PATH records from tracefs has
been changed/fixed/configured-out. This tmpfs Dentry issue happens on
both 4.8 and 4.11 kernels.
So the tmpfs is being unmounted within the time of a task that has taken
an anonymous parent audit_name and it appears that the dput in
audit_copy_inode() called from elsewhere had reset it to avoid
needlessly extending the life of this dentry.
I see two obvious ways to solve this:
- Return to freeing this dentry from audit_copy_inode()
- Add tmpfs to the list of filesystems to not create PATH records
I'm really not crazy aobut this second one unless I know why tmpfs is
generating calls with anonymous parents.
For reference, the rest of the call trace is:
dump_stack+0x85/0xc9
__warn+0xd1/0xf0
? d_genocide_kill+0x40/0x40
warn_slowpath_null+0x1d/0x20
umount_check+0x99/0xc0
d_walk+0x10b/0x580
? do_one_tree+0x26/0x40
do_one_tree+0x26/0x40
shrink_dcache_for_umount+0x5d/0xd0
generic_shutdown_super+0x1f/0xf0
kill_litter_super+0x29/0x40
deactivate_locked_super+0x43/0x70
deactivate_super+0x88/0xa0
cleanup_mnt+0x8e/0xe0
__cleanup_mnt+0x12/0x20
task_work_run+0x83/0xc0
do_exit+0x45c/0xfe0
? syscall_trace_enter+0x2e4/0x400
do_group_exit+0x68/0xe0
SyS_exit_group+0x14/0x20
do_syscall_64+0x82/0x270
entry_SYSCALL64_slow_path+0x25/0x25
> > I'm assuming you are hinting at also using that dentry to compare the
> > audit_names entry, which I think it a bad idea since there could be
> > multiple paths to access a dentry. I did orignially have another patch
> > that would have tried to use that as well, which didn't seem to hurt,
> > but I didn't think was worth upstreaming.
>
> No, I wasn't thinking that, the dev/inode numbers should be sufficient
> in those cases I believe; I'm not sure the dentry would help us here.
>
> >> Also out of curiosity, why do we want to drop a dentry reference here
> >> if one already exists?
> >
> > I think we want to drop a dentry reference here because this inode child
> > could be a subsequent access to the same dentry with a full path,
> > removing the need to cache this dentry information in the first place.
>
> Related to my comment above from today ... what code path please?
So it appears that there is a code path that does free this dentry in a
timely fashion to avoid needlessly extending its life and simply leaving
it there until the audit_context is freed is too long.
> >> > @@ -1925,6 +1930,17 @@ 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;
> >> > +
> >> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> >> > + if (!fullpath)
> >> > + return;
> >>
> >> I'm wondering if there is some value in still emitting the record if
> >> the kmalloc() fails, just with the name field set as the unset "?"
> >> value, e.g. "name=?". Thoughts?
> >
> > Possibly. We've got much bigger problems if that happens, but this
> > sounds like a good defensive coding approach. I'm even tempted to call
> > audit_panic().
>
> No audit_panic(). We've still got good information that we can
> record, e.g. dev/inode numbers; let's just print "name=?" and go on
> our way recording the rest of the information. This is in keeping
> with the current audit_log_name() error handling.
>
> At the very least you need to clean up here instead of just returning.
> As the patch currently stands I believe this will end up leaking an
> audit_buffer.
This has been fixed along with a fullpath kmalloc leak.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-08-11 6:36 ` Richard Guy Briggs
@ 2017-08-14 4:32 ` Richard Guy Briggs
0 siblings, 0 replies; 22+ messages in thread
From: Richard Guy Briggs @ 2017-08-14 4:32 UTC (permalink / raw)
To: Paul Moore
Cc: Greg Kroah-Hartman, linux-kernel, Steven Rostedt, linux-audit,
Al Viro, Ingo Molnar
On 2017-08-11 02:36, Richard Guy Briggs wrote:
> On 2017-06-28 15:03, Paul Moore wrote:
> > On Tue, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2017-05-30 17:21, Paul Moore wrote:
> > >> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > ...
> >
> > >> > diff --git a/kernel/audit.c b/kernel/audit.c
> > >> > index 25dd70a..7d83c5a 100644
> > >> > --- a/kernel/audit.c
> > >> > +++ b/kernel/audit.c
> > >> > @@ -66,6 +66,7 @@
> > >> > #include <linux/freezer.h>
> > >> > #include <linux/pid_namespace.h>
> > >> > #include <net/netns/generic.h>
> > >> > +#include <linux/dcache.h>
> > >> >
> > >> > #include "audit.h"
> > >> >
> > >> > @@ -1884,6 +1885,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;
> > >> > + }
> > >>
> > >> Out of curiosity, what terrible things happen if we take a reference
> > >> to a non-NULL dentry passed to audit_copy_inode() and store it in
> > >> name->dentry? Does performance tank?
> > >
> > > Interesting idea. Right now it is optimized to only take a reference to
> > > the dentry's parent dentry in the case we're handed an anonymous entry.
> > > Most of the time it will never be used even though we invest in the
> > > overhead of taking a reference count. Besides, __audit_inode_child()
> > > hands in a NULL for the dentry parameter to audit_copy_inode().
> >
> > [NOTE: audit_copy_inode() hands a NULL dentry only in the anonymous parent case]
> >
> > I believe I was just thinking of less conditional handling, especially
> > when reference counts are concerned. I'm just trying to limit future
> > headaches, but I suspect the perf cost would be problematic, and as
> > you point out, there is no *need* for the majority of cases.
> >
> > Looking at this again today, why would we want to clear name->dentry
> > in audit_copy_inode() if it is already set? Does that ever happen?
> > I'm not sure it does ...
>
> Ok, I just tried re-writing that part to dput that dentry only for the
> child of an anonymous parent rather than whenever audit_copy_inode() was
> called and I ended up with a:
>
> BUG: Dentry ffff8800338f1dc0{i=369a,n=stderr} still in use (1) [unmount of tmpfs tmpfs]
> WARNING: CPU: 0 PID: 387 at fs/dcache.c:1445 umount_check+0x99/0xc0
>
> This was after rebasing on a recent audit-next based on 4.11 rather than
> the previous based on a 4.8 audit-next. Something else changed in the
> kernel or kernel config between these two points. I was getting some
> "INFO: suspicious RCU usage." warnings in idmap for NFS on the earlier
> kernel and that is no longer happenning, and I'm no longer getting any
> tracefs audit PATH entries in the more recent kernel which suggests that
> whatever was causing the anonymous parent PATH records from tracefs has
> been changed/fixed/configured-out. This tmpfs Dentry issue happens on
> both 4.8 and 4.11 kernels.
Minor correction here: I'm still getting tracefs PATH records on 4.11,
so whatever happenned in that test appears to be a test procedure error.
> So the tmpfs is being unmounted within the time of a task that has taken
> an anonymous parent audit_name and it appears that the dput in
> audit_copy_inode() called from elsewhere had reset it to avoid
> needlessly extending the life of this dentry.
>
> I see two obvious ways to solve this:
> - Return to freeing this dentry from audit_copy_inode()
> - Add tmpfs to the list of filesystems to not create PATH records
>
> I'm really not crazy aobut this second one unless I know why tmpfs is
> generating calls with anonymous parents.
>
> For reference, the rest of the call trace is:
> dump_stack+0x85/0xc9
> __warn+0xd1/0xf0
> ? d_genocide_kill+0x40/0x40
> warn_slowpath_null+0x1d/0x20
> umount_check+0x99/0xc0
> d_walk+0x10b/0x580
> ? do_one_tree+0x26/0x40
> do_one_tree+0x26/0x40
> shrink_dcache_for_umount+0x5d/0xd0
> generic_shutdown_super+0x1f/0xf0
> kill_litter_super+0x29/0x40
> deactivate_locked_super+0x43/0x70
> deactivate_super+0x88/0xa0
> cleanup_mnt+0x8e/0xe0
> __cleanup_mnt+0x12/0x20
> task_work_run+0x83/0xc0
> do_exit+0x45c/0xfe0
> ? syscall_trace_enter+0x2e4/0x400
> do_group_exit+0x68/0xe0
> SyS_exit_group+0x14/0x20
> do_syscall_64+0x82/0x270
> entry_SYSCALL64_slow_path+0x25/0x25
>
> > > I'm assuming you are hinting at also using that dentry to compare the
> > > audit_names entry, which I think it a bad idea since there could be
> > > multiple paths to access a dentry. I did orignially have another patch
> > > that would have tried to use that as well, which didn't seem to hurt,
> > > but I didn't think was worth upstreaming.
> >
> > No, I wasn't thinking that, the dev/inode numbers should be sufficient
> > in those cases I believe; I'm not sure the dentry would help us here.
> >
> > >> Also out of curiosity, why do we want to drop a dentry reference here
> > >> if one already exists?
> > >
> > > I think we want to drop a dentry reference here because this inode child
> > > could be a subsequent access to the same dentry with a full path,
> > > removing the need to cache this dentry information in the first place.
> >
> > Related to my comment above from today ... what code path please?
>
> So it appears that there is a code path that does free this dentry in a
> timely fashion to avoid needlessly extending its life and simply leaving
> it there until the audit_context is freed is too long.
>
> > >> > @@ -1925,6 +1930,17 @@ 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;
> > >> > +
> > >> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > >> > + if (!fullpath)
> > >> > + return;
> > >>
> > >> I'm wondering if there is some value in still emitting the record if
> > >> the kmalloc() fails, just with the name field set as the unset "?"
> > >> value, e.g. "name=?". Thoughts?
> > >
> > > Possibly. We've got much bigger problems if that happens, but this
> > > sounds like a good defensive coding approach. I'm even tempted to call
> > > audit_panic().
> >
> > No audit_panic(). We've still got good information that we can
> > record, e.g. dev/inode numbers; let's just print "name=?" and go on
> > our way recording the rest of the information. This is in keeping
> > with the current audit_log_name() error handling.
> >
> > At the very least you need to clean up here instead of just returning.
> > As the patch currently stands I believe this will end up leaking an
> > audit_buffer.
>
> This has been fixed along with a fullpath kmalloc leak.
>
> > paul moore
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-08-14 4:32 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 9:21 [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
2017-04-04 9:21 ` [PATCH ALT4 V2 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
2017-04-04 10:02 ` Richard Guy Briggs
2017-04-04 10:02 ` Richard Guy Briggs
2017-05-30 21:30 ` Paul Moore
2017-06-27 20:45 ` Richard Guy Briggs
2017-06-28 19:08 ` Paul Moore
2017-06-28 21:25 ` Richard Guy Briggs
2017-06-28 21:25 ` Richard Guy Briggs
2017-04-04 21:19 ` [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
2017-04-05 4:18 ` Richard Guy Briggs
2017-04-05 4:18 ` Richard Guy Briggs
2017-05-30 21:21 ` Paul Moore
2017-06-27 21:11 ` Richard Guy Briggs
2017-06-27 21:11 ` Richard Guy Briggs
2017-06-28 19:03 ` Paul Moore
2017-06-29 21:21 ` Richard Guy Briggs
2017-06-29 23:58 ` Steven Rostedt
2017-06-30 13:07 ` Richard Guy Briggs
2017-06-30 13:07 ` Richard Guy Briggs
2017-08-11 6:36 ` Richard Guy Briggs
2017-08-14 4:32 ` Richard Guy Briggs
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.