* [PATCH ghak100 V2 0/2] audit: avoid umount hangs on missing mount
@ 2019-01-23 18:34 Richard Guy Briggs
2019-01-23 18:34 ` [PATCH ghak100 V2 1/2] audit: more filter PATH records keyed on filesystem magic Richard Guy Briggs
2019-01-23 18:35 ` [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount Richard Guy Briggs
0 siblings, 2 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2019-01-23 18:34 UTC (permalink / raw)
To: linux-fsdevel, viro, LKML, Linux-Audit Mailing List
Cc: Paul Moore, Steve Grubb, Eric Paris, Richard Guy Briggs
On user and remote filesystems, a forced umount can still hang due to
attemting to fetch the fcaps of a mounted filesystem that is no longer
available.
These two patches take different approaches to address this, one by
avoiding the lookup when the MNT_FORCE flag is included, the other by
providing a method to filter out auditing specified types of filesystems.
This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS or any
other userspace or remote filesystem.
Arguably the better way to address this issue is to avoid auditing
processes that touch removable filesystems.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100
Passes audit-testsuite including ghak100 branch.
Changelog:
v2:
- rebase on v5.0-rc1 audit/next
- refactor 3 levels of *if* indentation down to 1 incl. orig
- rename LOOKUP_NO_REVAL to LOOKUP_NO_EVAL to avoid existing usage
conflict
- don't depend on MNT_FORCE
- rename AUDIT_INODE_NOREVAL to AUDIT_INODE_NOREVAL to be consistent
- rename lflags to flags and flags to aflags
- document LOOKUP_ flags
- signal cap_* values unknown and set cap_* fields to "?" indicating so
Richard Guy Briggs (2):
audit: more filter PATH records keyed on filesystem magic
audit: ignore fcaps on umount
fs/namei.c | 2 +-
fs/namespace.c | 2 ++
include/linux/audit.h | 15 ++++++++++-----
include/linux/namei.h | 3 +++
kernel/audit.c | 10 +++++++++-
kernel/audit.h | 2 +-
kernel/auditsc.c | 41 ++++++++++++++++++++++++++++++-----------
7 files changed, 56 insertions(+), 19 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH ghak100 V2 1/2] audit: more filter PATH records keyed on filesystem magic
2019-01-23 18:34 [PATCH ghak100 V2 0/2] audit: avoid umount hangs on missing mount Richard Guy Briggs
@ 2019-01-23 18:34 ` Richard Guy Briggs
2019-01-25 21:36 ` Paul Moore
2019-01-23 18:35 ` [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount Richard Guy Briggs
1 sibling, 1 reply; 13+ messages in thread
From: Richard Guy Briggs @ 2019-01-23 18:34 UTC (permalink / raw)
To: linux-fsdevel, viro, LKML, Linux-Audit Mailing List
Cc: Paul Moore, Steve Grubb, Eric Paris, Richard Guy Briggs
Like commit 42d5e37654e4 ("audit: filter PATH records keyed on
filesystem magic") that addresses
https://github.com/linux-audit/audit-kernel/issues/8
Any user or remote filesystem could become unavailable and effectively
block on a forced unmount.
-a always,exit -S umount2 -F key=umount2
Provide a method to ignore these user and remote filesystems to prevent
them from being impossible to unmount.
Extend the "AUDIT_FILTER_FS" filter that uses the field type
AUDIT_FSTYPE keying off the filesystem 4-octet hexadecimal magic
identifier to filter specific filesystems to cover audit_inode() to address
this blockage.
An example rule would look like:
-a never,filesystem -F fstype=0x517B -F key=ignore_smb
-a never,filesystem -F fstype=0x6969 -F key=ignore_nfs
Arguably the better way to address this issue is to disable auditing
processes that touch removable filesystems.
Note: refactor __audit_inode_child() to remove two levels of if
indentation.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/auditsc.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b585ceb2f7a2..3d05d5fc6240 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1763,10 +1763,31 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
struct inode *inode = d_backing_inode(dentry);
struct audit_names *n;
bool parent = flags & AUDIT_INODE_PARENT;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;
if (!context->in_syscall)
return;
+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE
+ && audit_comparator(inode->i_sb->s_magic,
+ f->op, f->val)
+ && e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (!name)
goto out_alloc;
@@ -1875,14 +1896,12 @@ void __audit_inode_child(struct inode *parent,
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;
- }
- }
+ if (f->type == AUDIT_FSTYPE
+ && audit_comparator(parent->i_sb->s_magic,
+ f->op, f->val)
+ && e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
}
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-01-23 18:34 [PATCH ghak100 V2 0/2] audit: avoid umount hangs on missing mount Richard Guy Briggs
2019-01-23 18:34 ` [PATCH ghak100 V2 1/2] audit: more filter PATH records keyed on filesystem magic Richard Guy Briggs
@ 2019-01-23 18:35 ` Richard Guy Briggs
2019-01-25 21:45 ` Paul Moore
2019-02-01 20:42 ` Nathan Chancellor
1 sibling, 2 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2019-01-23 18:35 UTC (permalink / raw)
To: linux-fsdevel, viro, LKML, Linux-Audit Mailing List
Cc: Paul Moore, Steve Grubb, Eric Paris, Richard Guy Briggs
Don't fetch fcaps when umount2 is called to avoid a process hang while
it waits for the missing resource to (possibly never) re-appear.
Note the comment above user_path_mountpoint_at():
* A umount is a special case for path walking. We're not actually interested
* in the inode in this situation, and ESTALE errors can be a problem. We
* simply want track down the dentry and vfsmount attached at the mountpoint
* and avoid revalidating the last component.
This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
fs/namei.c | 2 +-
fs/namespace.c | 2 ++
include/linux/audit.h | 15 ++++++++++-----
include/linux/namei.h | 3 +++
kernel/audit.c | 10 +++++++++-
kernel/audit.h | 2 +-
kernel/auditsc.c | 6 +++---
7 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 914178cdbe94..87d7710a2e1d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2720,7 +2720,7 @@ int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
if (unlikely(error == -ESTALE))
error = path_mountpoint(&nd, flags | LOOKUP_REVAL, path);
if (likely(!error))
- audit_inode(name, path->dentry, 0);
+ audit_inode(name, path->dentry, flags & LOOKUP_NO_EVAL);
restore_nameidata();
putname(name);
return error;
diff --git a/fs/namespace.c b/fs/namespace.c
index a677b59efd74..e5de0e372df2 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1640,6 +1640,8 @@ int ksys_umount(char __user *name, int flags)
if (!(flags & UMOUNT_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ lookup_flags |= LOOKUP_NO_EVAL;
+
retval = user_path_mountpoint_at(AT_FDCWD, name, lookup_flags, &path);
if (retval)
goto out;
diff --git a/include/linux/audit.h b/include/linux/audit.h
index a625c29a2ea2..5d914b534536 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -25,6 +25,7 @@
#include <linux/sched.h>
#include <linux/ptrace.h>
+#include <linux/namei.h> /* LOOKUP_* */
#include <uapi/linux/audit.h>
#define AUDIT_INO_UNSET ((unsigned long)-1)
@@ -225,6 +226,7 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
#define AUDIT_INODE_PARENT 1 /* dentry represents the parent */
#define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden */
+#define AUDIT_INODE_NOEVAL 4 /* audit record incomplete */
extern void __audit_inode(struct filename *name, const struct dentry *dentry,
unsigned int flags);
extern void __audit_file(const struct file *);
@@ -285,12 +287,15 @@ static inline void audit_getname(struct filename *name)
}
static inline void audit_inode(struct filename *name,
const struct dentry *dentry,
- unsigned int parent) {
+ unsigned int flags) {
if (unlikely(!audit_dummy_context())) {
- unsigned int flags = 0;
- if (parent)
- flags |= AUDIT_INODE_PARENT;
- __audit_inode(name, dentry, flags);
+ unsigned int aflags = 0;
+
+ if (flags & LOOKUP_PARENT)
+ aflags |= AUDIT_INODE_PARENT;
+ if (flags & LOOKUP_NO_EVAL)
+ aflags |= AUDIT_INODE_NOEVAL;
+ __audit_inode(name, dentry, aflags);
}
}
static inline void audit_file(struct file *file)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..9138b4471dbf 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -24,6 +24,8 @@
* - internal "there are more path components" flag
* - dentry cache is untrusted; force a real lookup
* - suppress terminal automount
+ * - skip revalidation
+ * - don't fetch xattrs on audit_inode
*/
#define LOOKUP_FOLLOW 0x0001
#define LOOKUP_DIRECTORY 0x0002
@@ -33,6 +35,7 @@
#define LOOKUP_REVAL 0x0020
#define LOOKUP_RCU 0x0040
#define LOOKUP_NO_REVAL 0x0080
+#define LOOKUP_NO_EVAL 0x0100
/*
* Intent data
diff --git a/kernel/audit.c b/kernel/audit.c
index ca55ccb46b76..45d5131943eb 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2081,6 +2081,10 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
{
+ if (name->fcap_ver == -1) {
+ audit_log_format(ab, " cap_fe=? cap_fver=? cap_fp=? cap_fi=?");
+ return;
+ }
audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
audit_log_format(ab, " cap_fe=%d cap_fver=%x",
@@ -2111,7 +2115,7 @@ static inline int audit_copy_fcaps(struct audit_names *name,
/* Copy inode data into an audit_names. */
void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
- struct inode *inode)
+ struct inode *inode, unsigned int flags)
{
name->ino = inode->i_ino;
name->dev = inode->i_sb->s_dev;
@@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
+ name->fcap_ver = -1;
+ return;
+ }
audit_copy_fcaps(name, dentry);
}
diff --git a/kernel/audit.h b/kernel/audit.h
index 6ffb70575082..25ab276be8e5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -214,7 +214,7 @@ struct audit_context {
extern void audit_copy_inode(struct audit_names *name,
const struct dentry *dentry,
- struct inode *inode);
+ struct inode *inode, unsigned int flags);
extern void audit_log_cap(struct audit_buffer *ab, char *prefix,
kernel_cap_t *cap);
extern void audit_log_name(struct audit_context *context,
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 3d05d5fc6240..9a1257afc4d4 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1853,7 +1853,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
n->type = AUDIT_TYPE_NORMAL;
}
handle_path(dentry);
- audit_copy_inode(n, dentry, inode);
+ audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL);
}
void __audit_file(const struct file *file)
@@ -1952,7 +1952,7 @@ void __audit_inode_child(struct inode *parent,
n = audit_alloc_name(context, AUDIT_TYPE_PARENT);
if (!n)
return;
- audit_copy_inode(n, NULL, parent);
+ audit_copy_inode(n, NULL, parent, 0);
}
if (!found_child) {
@@ -1971,7 +1971,7 @@ void __audit_inode_child(struct inode *parent,
}
if (inode)
- audit_copy_inode(found_child, dentry, inode);
+ audit_copy_inode(found_child, dentry, inode, 0);
else
found_child->ino = AUDIT_INO_UNSET;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 1/2] audit: more filter PATH records keyed on filesystem magic
2019-01-23 18:34 ` [PATCH ghak100 V2 1/2] audit: more filter PATH records keyed on filesystem magic Richard Guy Briggs
@ 2019-01-25 21:36 ` Paul Moore
0 siblings, 0 replies; 13+ messages in thread
From: Paul Moore @ 2019-01-25 21:36 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-fsdevel, viro, LKML, Linux-Audit Mailing List, Steve Grubb,
Eric Paris
On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Like commit 42d5e37654e4 ("audit: filter PATH records keyed on
> filesystem magic") that addresses
> https://github.com/linux-audit/audit-kernel/issues/8
>
> Any user or remote filesystem could become unavailable and effectively
> block on a forced unmount.
>
> -a always,exit -S umount2 -F key=umount2
>
> Provide a method to ignore these user and remote filesystems to prevent
> them from being impossible to unmount.
>
> Extend the "AUDIT_FILTER_FS" filter that uses the field type
> AUDIT_FSTYPE keying off the filesystem 4-octet hexadecimal magic
> identifier to filter specific filesystems to cover audit_inode() to address
> this blockage.
>
> An example rule would look like:
> -a never,filesystem -F fstype=0x517B -F key=ignore_smb
> -a never,filesystem -F fstype=0x6969 -F key=ignore_nfs
>
> Arguably the better way to address this issue is to disable auditing
> processes that touch removable filesystems.
>
> Note: refactor __audit_inode_child() to remove two levels of if
> indentation.
>
> Please see the github issue tracker
> https://github.com/linux-audit/audit-kernel/issues/100
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/auditsc.c | 35 +++++++++++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 8 deletions(-)
Thanks, merged.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b585ceb2f7a2..3d05d5fc6240 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1763,10 +1763,31 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
> struct inode *inode = d_backing_inode(dentry);
> struct audit_names *n;
> bool parent = flags & AUDIT_INODE_PARENT;
> + struct audit_entry *e;
> + struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
> + int i;
>
> if (!context->in_syscall)
> return;
>
> + rcu_read_lock();
> + if (!list_empty(list)) {
> + list_for_each_entry_rcu(e, list, list) {
> + for (i = 0; i < e->rule.field_count; i++) {
> + struct audit_field *f = &e->rule.fields[i];
> +
> + if (f->type == AUDIT_FSTYPE
> + && audit_comparator(inode->i_sb->s_magic,
> + f->op, f->val)
> + && e->rule.action == AUDIT_NEVER) {
> + rcu_read_unlock();
> + return;
> + }
> + }
> + }
> + }
> + rcu_read_unlock();
> +
> if (!name)
> goto out_alloc;
>
> @@ -1875,14 +1896,12 @@ void __audit_inode_child(struct inode *parent,
> 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;
> - }
> - }
> + if (f->type == AUDIT_FSTYPE
> + && audit_comparator(parent->i_sb->s_magic,
> + f->op, f->val)
> + && e->rule.action == AUDIT_NEVER) {
> + rcu_read_unlock();
> + return;
> }
> }
> }
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-01-23 18:35 ` [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount Richard Guy Briggs
@ 2019-01-25 21:45 ` Paul Moore
2019-01-25 22:27 ` Richard Guy Briggs
2019-02-01 20:42 ` Nathan Chancellor
1 sibling, 1 reply; 13+ messages in thread
From: Paul Moore @ 2019-01-25 21:45 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-fsdevel, viro, LKML, Linux-Audit Mailing List, Steve Grubb,
Eric Paris
On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Don't fetch fcaps when umount2 is called to avoid a process hang while
> it waits for the missing resource to (possibly never) re-appear.
>
> Note the comment above user_path_mountpoint_at():
> * A umount is a special case for path walking. We're not actually interested
> * in the inode in this situation, and ESTALE errors can be a problem. We
> * simply want track down the dentry and vfsmount attached at the mountpoint
> * and avoid revalidating the last component.
>
> This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
>
> Please see the github issue tracker
> https://github.com/linux-audit/audit-kernel/issues/100
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/namei.c | 2 +-
> fs/namespace.c | 2 ++
> include/linux/audit.h | 15 ++++++++++-----
> include/linux/namei.h | 3 +++
> kernel/audit.c | 10 +++++++++-
> kernel/audit.h | 2 +-
> kernel/auditsc.c | 6 +++---
> 7 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 914178cdbe94..87d7710a2e1d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2720,7 +2720,7 @@ int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
> if (unlikely(error == -ESTALE))
> error = path_mountpoint(&nd, flags | LOOKUP_REVAL, path);
> if (likely(!error))
> - audit_inode(name, path->dentry, 0);
> + audit_inode(name, path->dentry, flags & LOOKUP_NO_EVAL);
> restore_nameidata();
> putname(name);
> return error;
> diff --git a/fs/namespace.c b/fs/namespace.c
> index a677b59efd74..e5de0e372df2 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1640,6 +1640,8 @@ int ksys_umount(char __user *name, int flags)
> if (!(flags & UMOUNT_NOFOLLOW))
> lookup_flags |= LOOKUP_FOLLOW;
>
> + lookup_flags |= LOOKUP_NO_EVAL;
> +
> retval = user_path_mountpoint_at(AT_FDCWD, name, lookup_flags, &path);
> if (retval)
> goto out;
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a625c29a2ea2..5d914b534536 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -25,6 +25,7 @@
>
> #include <linux/sched.h>
> #include <linux/ptrace.h>
> +#include <linux/namei.h> /* LOOKUP_* */
> #include <uapi/linux/audit.h>
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> @@ -225,6 +226,7 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>
> #define AUDIT_INODE_PARENT 1 /* dentry represents the parent */
> #define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden */
> +#define AUDIT_INODE_NOEVAL 4 /* audit record incomplete */
> extern void __audit_inode(struct filename *name, const struct dentry *dentry,
> unsigned int flags);
> extern void __audit_file(const struct file *);
> @@ -285,12 +287,15 @@ static inline void audit_getname(struct filename *name)
> }
> static inline void audit_inode(struct filename *name,
> const struct dentry *dentry,
> - unsigned int parent) {
> + unsigned int flags) {
> if (unlikely(!audit_dummy_context())) {
> - unsigned int flags = 0;
> - if (parent)
> - flags |= AUDIT_INODE_PARENT;
> - __audit_inode(name, dentry, flags);
> + unsigned int aflags = 0;
> +
> + if (flags & LOOKUP_PARENT)
> + aflags |= AUDIT_INODE_PARENT;
> + if (flags & LOOKUP_NO_EVAL)
> + aflags |= AUDIT_INODE_NOEVAL;
> + __audit_inode(name, dentry, aflags);
> }
> }
> static inline void audit_file(struct file *file)
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index a78606e8e3df..9138b4471dbf 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -24,6 +24,8 @@
> * - internal "there are more path components" flag
> * - dentry cache is untrusted; force a real lookup
> * - suppress terminal automount
> + * - skip revalidation
> + * - don't fetch xattrs on audit_inode
> */
> #define LOOKUP_FOLLOW 0x0001
> #define LOOKUP_DIRECTORY 0x0002
> @@ -33,6 +35,7 @@
> #define LOOKUP_REVAL 0x0020
> #define LOOKUP_RCU 0x0040
> #define LOOKUP_NO_REVAL 0x0080
> +#define LOOKUP_NO_EVAL 0x0100
>
> /*
> * Intent data
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ca55ccb46b76..45d5131943eb 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2081,6 +2081,10 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
>
> static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
> {
> + if (name->fcap_ver == -1) {
> + audit_log_format(ab, " cap_fe=? cap_fver=? cap_fp=? cap_fi=?");
> + return;
> + }
> audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
> audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
> audit_log_format(ab, " cap_fe=%d cap_fver=%x",
> @@ -2111,7 +2115,7 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>
> /* Copy inode data into an audit_names. */
> void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> - struct inode *inode)
> + struct inode *inode, unsigned int flags)
> {
> name->ino = inode->i_ino;
> name->dev = inode->i_sb->s_dev;
> @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> + name->fcap_ver = -1;
> + return;
> + }
I'm not in love with the -1 sentinel, especially without a cast in
front of it (style nit I suppose), but I love the idea of adding
another flag to audit_names even more. Since it looks like the
version portion of cpu_vfs_cap_data.magic_etc is limited to eight bits
I suppose we could break up fcap_ver and use some of the 32 bits for
our own flags. Not sure I like that option any more than the others.
I'm also beginning to wonder why we don't just include the fcaps
version info in the audit_cap_data struct; I can't think of a reason
why you would ever want to know one without the other. Thoughts?
> audit_copy_fcaps(name, dentry);
> }
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 6ffb70575082..25ab276be8e5 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -214,7 +214,7 @@ struct audit_context {
>
> extern void audit_copy_inode(struct audit_names *name,
> const struct dentry *dentry,
> - struct inode *inode);
> + struct inode *inode, unsigned int flags);
> extern void audit_log_cap(struct audit_buffer *ab, char *prefix,
> kernel_cap_t *cap);
> extern void audit_log_name(struct audit_context *context,
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 3d05d5fc6240..9a1257afc4d4 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1853,7 +1853,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
> n->type = AUDIT_TYPE_NORMAL;
> }
> handle_path(dentry);
> - audit_copy_inode(n, dentry, inode);
> + audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL);
> }
>
> void __audit_file(const struct file *file)
> @@ -1952,7 +1952,7 @@ void __audit_inode_child(struct inode *parent,
> n = audit_alloc_name(context, AUDIT_TYPE_PARENT);
> if (!n)
> return;
> - audit_copy_inode(n, NULL, parent);
> + audit_copy_inode(n, NULL, parent, 0);
> }
>
> if (!found_child) {
> @@ -1971,7 +1971,7 @@ void __audit_inode_child(struct inode *parent,
> }
>
> if (inode)
> - audit_copy_inode(found_child, dentry, inode);
> + audit_copy_inode(found_child, dentry, inode, 0);
> else
> found_child->ino = AUDIT_INO_UNSET;
> }
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-01-25 21:45 ` Paul Moore
@ 2019-01-25 22:27 ` Richard Guy Briggs
2019-01-28 23:25 ` Paul Moore
0 siblings, 1 reply; 13+ messages in thread
From: Richard Guy Briggs @ 2019-01-25 22:27 UTC (permalink / raw)
To: Paul Moore; +Cc: LKML, Linux-Audit Mailing List, viro, linux-fsdevel
On 2019-01-25 16:45, Paul Moore wrote:
> On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > it waits for the missing resource to (possibly never) re-appear.
> >
> > Note the comment above user_path_mountpoint_at():
> > * A umount is a special case for path walking. We're not actually interested
> > * in the inode in this situation, and ESTALE errors can be a problem. We
> > * simply want track down the dentry and vfsmount attached at the mountpoint
> > * and avoid revalidating the last component.
> >
> > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> >
> > Please see the github issue tracker
> > https://github.com/linux-audit/audit-kernel/issues/100
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > fs/namei.c | 2 +-
> > fs/namespace.c | 2 ++
> > include/linux/audit.h | 15 ++++++++++-----
> > include/linux/namei.h | 3 +++
> > kernel/audit.c | 10 +++++++++-
> > kernel/audit.h | 2 +-
> > kernel/auditsc.c | 6 +++---
> > 7 files changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 914178cdbe94..87d7710a2e1d 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2720,7 +2720,7 @@ int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
> > if (unlikely(error == -ESTALE))
> > error = path_mountpoint(&nd, flags | LOOKUP_REVAL, path);
> > if (likely(!error))
> > - audit_inode(name, path->dentry, 0);
> > + audit_inode(name, path->dentry, flags & LOOKUP_NO_EVAL);
> > restore_nameidata();
> > putname(name);
> > return error;
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index a677b59efd74..e5de0e372df2 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -1640,6 +1640,8 @@ int ksys_umount(char __user *name, int flags)
> > if (!(flags & UMOUNT_NOFOLLOW))
> > lookup_flags |= LOOKUP_FOLLOW;
> >
> > + lookup_flags |= LOOKUP_NO_EVAL;
> > +
> > retval = user_path_mountpoint_at(AT_FDCWD, name, lookup_flags, &path);
> > if (retval)
> > goto out;
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index a625c29a2ea2..5d914b534536 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -25,6 +25,7 @@
> >
> > #include <linux/sched.h>
> > #include <linux/ptrace.h>
> > +#include <linux/namei.h> /* LOOKUP_* */
> > #include <uapi/linux/audit.h>
> >
> > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > @@ -225,6 +226,7 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> >
> > #define AUDIT_INODE_PARENT 1 /* dentry represents the parent */
> > #define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden */
> > +#define AUDIT_INODE_NOEVAL 4 /* audit record incomplete */
> > extern void __audit_inode(struct filename *name, const struct dentry *dentry,
> > unsigned int flags);
> > extern void __audit_file(const struct file *);
> > @@ -285,12 +287,15 @@ static inline void audit_getname(struct filename *name)
> > }
> > static inline void audit_inode(struct filename *name,
> > const struct dentry *dentry,
> > - unsigned int parent) {
> > + unsigned int flags) {
> > if (unlikely(!audit_dummy_context())) {
> > - unsigned int flags = 0;
> > - if (parent)
> > - flags |= AUDIT_INODE_PARENT;
> > - __audit_inode(name, dentry, flags);
> > + unsigned int aflags = 0;
> > +
> > + if (flags & LOOKUP_PARENT)
> > + aflags |= AUDIT_INODE_PARENT;
> > + if (flags & LOOKUP_NO_EVAL)
> > + aflags |= AUDIT_INODE_NOEVAL;
> > + __audit_inode(name, dentry, aflags);
> > }
> > }
> > static inline void audit_file(struct file *file)
> > diff --git a/include/linux/namei.h b/include/linux/namei.h
> > index a78606e8e3df..9138b4471dbf 100644
> > --- a/include/linux/namei.h
> > +++ b/include/linux/namei.h
> > @@ -24,6 +24,8 @@
> > * - internal "there are more path components" flag
> > * - dentry cache is untrusted; force a real lookup
> > * - suppress terminal automount
> > + * - skip revalidation
> > + * - don't fetch xattrs on audit_inode
> > */
> > #define LOOKUP_FOLLOW 0x0001
> > #define LOOKUP_DIRECTORY 0x0002
> > @@ -33,6 +35,7 @@
> > #define LOOKUP_REVAL 0x0020
> > #define LOOKUP_RCU 0x0040
> > #define LOOKUP_NO_REVAL 0x0080
> > +#define LOOKUP_NO_EVAL 0x0100
> >
> > /*
> > * Intent data
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index ca55ccb46b76..45d5131943eb 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2081,6 +2081,10 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
> >
> > static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
> > {
> > + if (name->fcap_ver == -1) {
> > + audit_log_format(ab, " cap_fe=? cap_fver=? cap_fp=? cap_fi=?");
> > + return;
> > + }
> > audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
> > audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
> > audit_log_format(ab, " cap_fe=%d cap_fver=%x",
> > @@ -2111,7 +2115,7 @@ static inline int audit_copy_fcaps(struct audit_names *name,
> >
> > /* Copy inode data into an audit_names. */
> > void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > - struct inode *inode)
> > + struct inode *inode, unsigned int flags)
> > {
> > name->ino = inode->i_ino;
> > name->dev = inode->i_sb->s_dev;
> > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> > + name->fcap_ver = -1;
> > + return;
> > + }
>
> I'm not in love with the -1 sentinel, especially without a cast in
> front of it (style nit I suppose), but I love the idea of adding
> another flag to audit_names even more. Since it looks like the
> version portion of cpu_vfs_cap_data.magic_etc is limited to eight bits
> I suppose we could break up fcap_ver and use some of the 32 bits for
> our own flags. Not sure I like that option any more than the others.
Would you prefer 0xFFFFFFFF, or 0xFF (assuming it somehow needs to be
compatible with the shifted revision mask), or even 0xFFFFFF00 to avoid
using those 8 bits?
As far as I know, only one bit of the original 24 magic_etc flag bits is
used and that is for the fE (file Effective) bit.
> I'm also beginning to wonder why we don't just include the fcaps
> version info in the audit_cap_data struct; I can't think of a reason
> why you would ever want to know one without the other. Thoughts?
I wondered that myself... struct audit_cap_data is also used in
audit_context.capset where it has no need of the version since it is
storing process caps (and the effective field is a union with fE) and in
audit_context.aux where it is chained in as audit_aux_data_bprm_fcaps
with three instances of audit_cap_data but only one version.
We could just keep fcap_ver and fE in the original magic_etc and use
a high order bit of the VFS_CAP_FLAGS_MASK...
Is it worth changing?
> > audit_copy_fcaps(name, dentry);
> > }
> >
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 6ffb70575082..25ab276be8e5 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -214,7 +214,7 @@ struct audit_context {
> >
> > extern void audit_copy_inode(struct audit_names *name,
> > const struct dentry *dentry,
> > - struct inode *inode);
> > + struct inode *inode, unsigned int flags);
> > extern void audit_log_cap(struct audit_buffer *ab, char *prefix,
> > kernel_cap_t *cap);
> > extern void audit_log_name(struct audit_context *context,
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 3d05d5fc6240..9a1257afc4d4 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1853,7 +1853,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
> > n->type = AUDIT_TYPE_NORMAL;
> > }
> > handle_path(dentry);
> > - audit_copy_inode(n, dentry, inode);
> > + audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL);
> > }
> >
> > void __audit_file(const struct file *file)
> > @@ -1952,7 +1952,7 @@ void __audit_inode_child(struct inode *parent,
> > n = audit_alloc_name(context, AUDIT_TYPE_PARENT);
> > if (!n)
> > return;
> > - audit_copy_inode(n, NULL, parent);
> > + audit_copy_inode(n, NULL, parent, 0);
> > }
> >
> > if (!found_child) {
> > @@ -1971,7 +1971,7 @@ void __audit_inode_child(struct inode *parent,
> > }
> >
> > if (inode)
> > - audit_copy_inode(found_child, dentry, inode);
> > + audit_copy_inode(found_child, dentry, inode, 0);
> > else
> > found_child->ino = AUDIT_INO_UNSET;
> > }
> > --
> > 1.8.3.1
> >
>
>
> --
> paul moore
> www.paul-moore.com
>
> --
> 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] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-01-25 22:27 ` Richard Guy Briggs
@ 2019-01-28 23:25 ` Paul Moore
2019-01-31 2:20 ` Paul Moore
0 siblings, 1 reply; 13+ messages in thread
From: Paul Moore @ 2019-01-28 23:25 UTC (permalink / raw)
To: Richard Guy Briggs, linux-fsdevel, viro; +Cc: LKML, Linux-Audit Mailing List
On Fri, Jan 25, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-01-25 16:45, Paul Moore wrote:
> > On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > it waits for the missing resource to (possibly never) re-appear.
> > >
> > > Note the comment above user_path_mountpoint_at():
> > > * A umount is a special case for path walking. We're not actually interested
> > > * in the inode in this situation, and ESTALE errors can be a problem. We
> > > * simply want track down the dentry and vfsmount attached at the mountpoint
> > > * and avoid revalidating the last component.
> > >
> > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > >
> > > Please see the github issue tracker
> > > https://github.com/linux-audit/audit-kernel/issues/100
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > fs/namei.c | 2 +-
> > > fs/namespace.c | 2 ++
> > > include/linux/audit.h | 15 ++++++++++-----
> > > include/linux/namei.h | 3 +++
> > > kernel/audit.c | 10 +++++++++-
> > > kernel/audit.h | 2 +-
> > > kernel/auditsc.c | 6 +++---
> > > 7 files changed, 29 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 914178cdbe94..87d7710a2e1d 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -2720,7 +2720,7 @@ int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
> > > if (unlikely(error == -ESTALE))
> > > error = path_mountpoint(&nd, flags | LOOKUP_REVAL, path);
> > > if (likely(!error))
> > > - audit_inode(name, path->dentry, 0);
> > > + audit_inode(name, path->dentry, flags & LOOKUP_NO_EVAL);
> > > restore_nameidata();
> > > putname(name);
> > > return error;
> > > diff --git a/fs/namespace.c b/fs/namespace.c
> > > index a677b59efd74..e5de0e372df2 100644
> > > --- a/fs/namespace.c
> > > +++ b/fs/namespace.c
> > > @@ -1640,6 +1640,8 @@ int ksys_umount(char __user *name, int flags)
> > > if (!(flags & UMOUNT_NOFOLLOW))
> > > lookup_flags |= LOOKUP_FOLLOW;
> > >
> > > + lookup_flags |= LOOKUP_NO_EVAL;
> > > +
> > > retval = user_path_mountpoint_at(AT_FDCWD, name, lookup_flags, &path);
> > > if (retval)
> > > goto out;
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index a625c29a2ea2..5d914b534536 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -25,6 +25,7 @@
> > >
> > > #include <linux/sched.h>
> > > #include <linux/ptrace.h>
> > > +#include <linux/namei.h> /* LOOKUP_* */
> > > #include <uapi/linux/audit.h>
> > >
> > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > @@ -225,6 +226,7 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> > >
> > > #define AUDIT_INODE_PARENT 1 /* dentry represents the parent */
> > > #define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden */
> > > +#define AUDIT_INODE_NOEVAL 4 /* audit record incomplete */
> > > extern void __audit_inode(struct filename *name, const struct dentry *dentry,
> > > unsigned int flags);
> > > extern void __audit_file(const struct file *);
> > > @@ -285,12 +287,15 @@ static inline void audit_getname(struct filename *name)
> > > }
> > > static inline void audit_inode(struct filename *name,
> > > const struct dentry *dentry,
> > > - unsigned int parent) {
> > > + unsigned int flags) {
> > > if (unlikely(!audit_dummy_context())) {
> > > - unsigned int flags = 0;
> > > - if (parent)
> > > - flags |= AUDIT_INODE_PARENT;
> > > - __audit_inode(name, dentry, flags);
> > > + unsigned int aflags = 0;
> > > +
> > > + if (flags & LOOKUP_PARENT)
> > > + aflags |= AUDIT_INODE_PARENT;
> > > + if (flags & LOOKUP_NO_EVAL)
> > > + aflags |= AUDIT_INODE_NOEVAL;
> > > + __audit_inode(name, dentry, aflags);
> > > }
> > > }
> > > static inline void audit_file(struct file *file)
> > > diff --git a/include/linux/namei.h b/include/linux/namei.h
> > > index a78606e8e3df..9138b4471dbf 100644
> > > --- a/include/linux/namei.h
> > > +++ b/include/linux/namei.h
> > > @@ -24,6 +24,8 @@
> > > * - internal "there are more path components" flag
> > > * - dentry cache is untrusted; force a real lookup
> > > * - suppress terminal automount
> > > + * - skip revalidation
> > > + * - don't fetch xattrs on audit_inode
> > > */
> > > #define LOOKUP_FOLLOW 0x0001
> > > #define LOOKUP_DIRECTORY 0x0002
> > > @@ -33,6 +35,7 @@
> > > #define LOOKUP_REVAL 0x0020
> > > #define LOOKUP_RCU 0x0040
> > > #define LOOKUP_NO_REVAL 0x0080
> > > +#define LOOKUP_NO_EVAL 0x0100
> > >
> > > /*
> > > * Intent data
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index ca55ccb46b76..45d5131943eb 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -2081,6 +2081,10 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
> > >
> > > static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
> > > {
> > > + if (name->fcap_ver == -1) {
> > > + audit_log_format(ab, " cap_fe=? cap_fver=? cap_fp=? cap_fi=?");
> > > + return;
> > > + }
> > > audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
> > > audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
> > > audit_log_format(ab, " cap_fe=%d cap_fver=%x",
> > > @@ -2111,7 +2115,7 @@ static inline int audit_copy_fcaps(struct audit_names *name,
> > >
> > > /* Copy inode data into an audit_names. */
> > > void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > > - struct inode *inode)
> > > + struct inode *inode, unsigned int flags)
> > > {
> > > name->ino = inode->i_ino;
> > > name->dev = inode->i_sb->s_dev;
> > > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> > > + name->fcap_ver = -1;
> > > + return;
> > > + }
> >
> > I'm not in love with the -1 sentinel, especially without a cast in
> > front of it (style nit I suppose), but I love the idea of adding
> > another flag to audit_names even more. Since it looks like the
> > version portion of cpu_vfs_cap_data.magic_etc is limited to eight bits
> > I suppose we could break up fcap_ver and use some of the 32 bits for
> > our own flags. Not sure I like that option any more than the others.
>
> Would you prefer 0xFFFFFFFF, or 0xFF (assuming it somehow needs to be
> compatible with the shifted revision mask), or even 0xFFFFFF00 to avoid
> using those 8 bits?
>
> As far as I know, only one bit of the original 24 magic_etc flag bits is
> used and that is for the fE (file Effective) bit.
>
> > I'm also beginning to wonder why we don't just include the fcaps
> > version info in the audit_cap_data struct; I can't think of a reason
> > why you would ever want to know one without the other. Thoughts?
>
> I wondered that myself... struct audit_cap_data is also used in
> audit_context.capset where it has no need of the version since it is
> storing process caps (and the effective field is a union with fE) and in
> audit_context.aux where it is chained in as audit_aux_data_bprm_fcaps
> with three instances of audit_cap_data but only one version.
>
> We could just keep fcap_ver and fE in the original magic_etc and use
> a high order bit of the VFS_CAP_FLAGS_MASK...
>
> Is it worth changing?
Yeah, that is the question I was grappling with last week, and again
now. Although to be more specific, the question isn't really is it
worth changing, but rather what would be an improvement on this? I
can think of a few small things, but nothing that would warrant a
respin, especially since this all audit internal so we can change it
without much fuss if needed.
You posted this patch on the 23rd (last Wednesday), let's give the FS
folks another day or two to comment since it does touch their code.
If we don't see any objections by later this week I'll merge it into
audit/next.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-01-28 23:25 ` Paul Moore
@ 2019-01-31 2:20 ` Paul Moore
0 siblings, 0 replies; 13+ messages in thread
From: Paul Moore @ 2019-01-31 2:20 UTC (permalink / raw)
To: Richard Guy Briggs, linux-fsdevel, viro; +Cc: LKML, Linux-Audit Mailing List
On Mon, Jan 28, 2019 at 6:25 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jan 25, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-01-25 16:45, Paul Moore wrote:
> > > On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > > it waits for the missing resource to (possibly never) re-appear.
> > > >
> > > > Note the comment above user_path_mountpoint_at():
> > > > * A umount is a special case for path walking. We're not actually interested
> > > > * in the inode in this situation, and ESTALE errors can be a problem. We
> > > > * simply want track down the dentry and vfsmount attached at the mountpoint
> > > > * and avoid revalidating the last component.
> > > >
> > > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > > >
> > > > Please see the github issue tracker
> > > > https://github.com/linux-audit/audit-kernel/issues/100
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > fs/namei.c | 2 +-
> > > > fs/namespace.c | 2 ++
> > > > include/linux/audit.h | 15 ++++++++++-----
> > > > include/linux/namei.h | 3 +++
> > > > kernel/audit.c | 10 +++++++++-
> > > > kernel/audit.h | 2 +-
> > > > kernel/auditsc.c | 6 +++---
> > > > 7 files changed, 29 insertions(+), 11 deletions(-)
...
> You posted this patch on the 23rd (last Wednesday), let's give the FS
> folks another day or two to comment since it does touch their code.
> If we don't see any objections by later this week I'll merge it into
> audit/next.
I'm not seeing any objections from the fs folks, so I'm merging this
into audit/next.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-01-23 18:35 ` [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount Richard Guy Briggs
2019-01-25 21:45 ` Paul Moore
@ 2019-02-01 20:42 ` Nathan Chancellor
2019-02-01 21:05 ` Paul Moore
1 sibling, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2019-02-01 20:42 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-fsdevel, viro, LKML, Linux-Audit Mailing List, Paul Moore,
Steve Grubb, Eric Paris
On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> Don't fetch fcaps when umount2 is called to avoid a process hang while
> it waits for the missing resource to (possibly never) re-appear.
>
> Note the comment above user_path_mountpoint_at():
> * A umount is a special case for path walking. We're not actually interested
> * in the inode in this situation, and ESTALE errors can be a problem. We
> * simply want track down the dentry and vfsmount attached at the mountpoint
> * and avoid revalidating the last component.
>
> This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
>
> Please see the github issue tracker
> https://github.com/linux-audit/audit-kernel/issues/100
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/namei.c | 2 +-
> fs/namespace.c | 2 ++
> include/linux/audit.h | 15 ++++++++++-----
> include/linux/namei.h | 3 +++
> kernel/audit.c | 10 +++++++++-
> kernel/audit.h | 2 +-
> kernel/auditsc.c | 6 +++---
> 7 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 914178cdbe94..87d7710a2e1d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2720,7 +2720,7 @@ int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
> if (unlikely(error == -ESTALE))
> error = path_mountpoint(&nd, flags | LOOKUP_REVAL, path);
> if (likely(!error))
> - audit_inode(name, path->dentry, 0);
> + audit_inode(name, path->dentry, flags & LOOKUP_NO_EVAL);
> restore_nameidata();
> putname(name);
> return error;
> diff --git a/fs/namespace.c b/fs/namespace.c
> index a677b59efd74..e5de0e372df2 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1640,6 +1640,8 @@ int ksys_umount(char __user *name, int flags)
> if (!(flags & UMOUNT_NOFOLLOW))
> lookup_flags |= LOOKUP_FOLLOW;
>
> + lookup_flags |= LOOKUP_NO_EVAL;
> +
> retval = user_path_mountpoint_at(AT_FDCWD, name, lookup_flags, &path);
> if (retval)
> goto out;
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a625c29a2ea2..5d914b534536 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -25,6 +25,7 @@
>
> #include <linux/sched.h>
> #include <linux/ptrace.h>
> +#include <linux/namei.h> /* LOOKUP_* */
> #include <uapi/linux/audit.h>
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> @@ -225,6 +226,7 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>
> #define AUDIT_INODE_PARENT 1 /* dentry represents the parent */
> #define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden */
> +#define AUDIT_INODE_NOEVAL 4 /* audit record incomplete */
> extern void __audit_inode(struct filename *name, const struct dentry *dentry,
> unsigned int flags);
> extern void __audit_file(const struct file *);
> @@ -285,12 +287,15 @@ static inline void audit_getname(struct filename *name)
> }
> static inline void audit_inode(struct filename *name,
> const struct dentry *dentry,
> - unsigned int parent) {
> + unsigned int flags) {
> if (unlikely(!audit_dummy_context())) {
> - unsigned int flags = 0;
> - if (parent)
> - flags |= AUDIT_INODE_PARENT;
> - __audit_inode(name, dentry, flags);
> + unsigned int aflags = 0;
> +
> + if (flags & LOOKUP_PARENT)
> + aflags |= AUDIT_INODE_PARENT;
> + if (flags & LOOKUP_NO_EVAL)
> + aflags |= AUDIT_INODE_NOEVAL;
> + __audit_inode(name, dentry, aflags);
> }
> }
> static inline void audit_file(struct file *file)
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index a78606e8e3df..9138b4471dbf 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -24,6 +24,8 @@
> * - internal "there are more path components" flag
> * - dentry cache is untrusted; force a real lookup
> * - suppress terminal automount
> + * - skip revalidation
> + * - don't fetch xattrs on audit_inode
> */
> #define LOOKUP_FOLLOW 0x0001
> #define LOOKUP_DIRECTORY 0x0002
> @@ -33,6 +35,7 @@
> #define LOOKUP_REVAL 0x0020
> #define LOOKUP_RCU 0x0040
> #define LOOKUP_NO_REVAL 0x0080
> +#define LOOKUP_NO_EVAL 0x0100
>
> /*
> * Intent data
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ca55ccb46b76..45d5131943eb 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2081,6 +2081,10 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
>
> static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
> {
> + if (name->fcap_ver == -1) {
> + audit_log_format(ab, " cap_fe=? cap_fver=? cap_fp=? cap_fi=?");
> + return;
> + }
> audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
> audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
> audit_log_format(ab, " cap_fe=%d cap_fver=%x",
> @@ -2111,7 +2115,7 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>
> /* Copy inode data into an audit_names. */
> void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> - struct inode *inode)
> + struct inode *inode, unsigned int flags)
> {
> name->ino = inode->i_ino;
> name->dev = inode->i_sb->s_dev;
> @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
I don't know if this has been reported or if I am missing something but
on next-20190201, this line causes an error with arm allyesconfig (as
CONFIG_AUDITSYCALL doesn't get selected):
$ make -j$(nproc) ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- allyesconfig kernel/audit.o
HOSTCC scripts/basic/fixdep
HOSTCC scripts/kconfig/confdata.o
HOSTCC scripts/kconfig/conf.o
HOSTCC scripts/kconfig/expr.o
YACC scripts/kconfig/parser.tab.h
YACC scripts/kconfig/parser.tab.c
LEX scripts/kconfig/lexer.lex.c
HOSTCC scripts/kconfig/symbol.o
HOSTCC scripts/kconfig/preprocess.o
HOSTCC scripts/kconfig/parser.tab.o
HOSTCC scripts/kconfig/lexer.lex.o
HOSTLD scripts/kconfig/conf
scripts/kconfig/conf --allyesconfig Kconfig
#
# configuration written to .config
#
SYSHDR arch/arm/include/generated/uapi/asm/unistd-common.h
SYSHDR arch/arm/include/generated/uapi/asm/unistd-oabi.h
SYSHDR arch/arm/include/generated/uapi/asm/unistd-eabi.h
WRAP arch/arm/include/generated/uapi/asm/bitsperlong.h
WRAP arch/arm/include/generated/uapi/asm/ipcbuf.h
WRAP arch/arm/include/generated/uapi/asm/bpf_perf_event.h
WRAP arch/arm/include/generated/uapi/asm/errno.h
WRAP arch/arm/include/generated/uapi/asm/msgbuf.h
WRAP arch/arm/include/generated/uapi/asm/ioctl.h
WRAP arch/arm/include/generated/uapi/asm/sembuf.h
WRAP arch/arm/include/generated/uapi/asm/shmbuf.h
WRAP arch/arm/include/generated/uapi/asm/param.h
WRAP arch/arm/include/generated/uapi/asm/poll.h
WRAP arch/arm/include/generated/uapi/asm/resource.h
WRAP arch/arm/include/generated/uapi/asm/siginfo.h
WRAP arch/arm/include/generated/uapi/asm/termbits.h
WRAP arch/arm/include/generated/uapi/asm/socket.h
WRAP arch/arm/include/generated/uapi/asm/sockios.h
WRAP arch/arm/include/generated/uapi/asm/termios.h
UPD include/generated/uapi/linux/version.h
WRAP arch/arm/include/generated/asm/current.h
WRAP arch/arm/include/generated/asm/compat.h
WRAP arch/arm/include/generated/asm/emergency-restart.h
WRAP arch/arm/include/generated/asm/early_ioremap.h
WRAP arch/arm/include/generated/asm/exec.h
WRAP arch/arm/include/generated/asm/irq_regs.h
WRAP arch/arm/include/generated/asm/extable.h
WRAP arch/arm/include/generated/asm/kdebug.h
WRAP arch/arm/include/generated/asm/local64.h
WRAP arch/arm/include/generated/asm/msi.h
WRAP arch/arm/include/generated/asm/parport.h
WRAP arch/arm/include/generated/asm/local.h
WRAP arch/arm/include/generated/asm/segment.h
WRAP arch/arm/include/generated/asm/mm-arch-hooks.h
WRAP arch/arm/include/generated/asm/preempt.h
WRAP arch/arm/include/generated/asm/simd.h
WRAP arch/arm/include/generated/asm/rwsem.h
WRAP arch/arm/include/generated/asm/seccomp.h
WRAP arch/arm/include/generated/asm/serial.h
WRAP arch/arm/include/generated/asm/timex.h
WRAP arch/arm/include/generated/asm/trace_clock.h
WRAP arch/arm/include/generated/asm/sizes.h
HOSTCC scripts/dtc/dtc.o
HOSTCC scripts/dtc/fstree.o
HOSTCC scripts/dtc/flattree.o
HOSTCC scripts/dtc/data.o
HOSTCC scripts/dtc/livetree.o
HOSTCC scripts/dtc/treesource.o
HOSTCC scripts/dtc/srcpos.o
HOSTCC scripts/dtc/util.o
HOSTCC scripts/dtc/checks.o
LEX scripts/dtc/dtc-lexer.lex.c
YACC scripts/dtc/dtc-parser.tab.h
YACC scripts/dtc/dtc-parser.tab.c
HOSTCC scripts/dtc/yamltree.o
UPD include/config/kernel.release
UPD include/generated/utsrelease.h
HOSTCC scripts/dtc/dtc-parser.tab.o
HOSTCC scripts/dtc/dtc-lexer.lex.o
HOSTLD scripts/dtc/dtc
HOSTCC scripts/bin2c
HOSTCC scripts/kallsyms
HOSTCC scripts/pnmtologo
HOSTCC scripts/conmakehash
HOSTCC scripts/recordmcount
HOSTCC scripts/sortextable
HOSTCC scripts/sign-file
HOSTCC scripts/asn1_compiler
HOSTCC scripts/extract-cert
HOSTCC scripts/insert-sys-cert
HOSTCC scripts/genksyms/genksyms.o
YACC scripts/genksyms/parse.tab.c
YACC scripts/genksyms/parse.tab.h
LEX scripts/genksyms/lex.lex.c
HOSTCXX -fPIC scripts/gcc-plugins/latent_entropy_plugin.o
HOSTCXX -fPIC scripts/gcc-plugins/structleak_plugin.o
GENSEED scripts/gcc-plugins/randomize_layout_seed.h
HOSTCC scripts/selinux/genheaders/genheaders
HOSTCXX -fPIC scripts/gcc-plugins/arm_ssp_per_task_plugin.o
HOSTCC scripts/selinux/mdp/mdp
HOSTCC scripts/genksyms/parse.tab.o
HOSTCC scripts/genksyms/lex.lex.o
HOSTLD scripts/genksyms/genksyms
HOSTCXX -fPIC scripts/gcc-plugins/randomize_layout_plugin.o
HOSTLLD -shared scripts/gcc-plugins/structleak_plugin.so
HOSTLLD -shared scripts/gcc-plugins/latent_entropy_plugin.so
HOSTLLD -shared scripts/gcc-plugins/arm_ssp_per_task_plugin.so
HOSTLLD -shared scripts/gcc-plugins/randomize_layout_plugin.so
GEN arch/arm/include/generated/asm/mach-types.h
SYSNR arch/arm/include/generated/asm/unistd-nr.h
SYSTBL arch/arm/include/generated/calls-oabi.S
SYSTBL arch/arm/include/generated/calls-eabi.S
HOSTCC scripts/mod/mk_elfconfig
CC scripts/mod/devicetable-offsets.s
CC scripts/mod/empty.o
UPD scripts/mod/devicetable-offsets.h
MKELF scripts/mod/elfconfig.h
HOSTCC scripts/mod/modpost.o
HOSTCC scripts/mod/sumversion.o
HOSTCC scripts/mod/file2alias.o
HOSTLD scripts/mod/modpost
CC kernel/bounds.s
CALL scripts/atomic/check-atomics.sh
UPD include/generated/timeconst.h
UPD include/generated/bounds.h
CC arch/arm/kernel/asm-offsets.s
UPD include/generated/asm-offsets.h
CALL scripts/checksyscalls.sh
<stdin>:1469:2: warning: #warning syscall pidfd_send_signal not implemented [-Wcpp]
CC kernel/audit.o
kernel/audit.c: In function 'audit_copy_inode':
kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in this function); did you mean 'AUDIT_TYPE_NORMAL'?
if (flags & AUDIT_INODE_NOEVAL) {
^~~~~~~~~~~~~~~~~~
AUDIT_TYPE_NORMAL
kernel/audit.c:2130:14: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
make: *** [Makefile:296: __build_one_by_one] Error 2
> + name->fcap_ver = -1;
> + return;
> + }
> audit_copy_fcaps(name, dentry);
> }
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 6ffb70575082..25ab276be8e5 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -214,7 +214,7 @@ struct audit_context {
>
> extern void audit_copy_inode(struct audit_names *name,
> const struct dentry *dentry,
> - struct inode *inode);
> + struct inode *inode, unsigned int flags);
> extern void audit_log_cap(struct audit_buffer *ab, char *prefix,
> kernel_cap_t *cap);
> extern void audit_log_name(struct audit_context *context,
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 3d05d5fc6240..9a1257afc4d4 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1853,7 +1853,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
> n->type = AUDIT_TYPE_NORMAL;
> }
> handle_path(dentry);
> - audit_copy_inode(n, dentry, inode);
> + audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL);
> }
>
> void __audit_file(const struct file *file)
> @@ -1952,7 +1952,7 @@ void __audit_inode_child(struct inode *parent,
> n = audit_alloc_name(context, AUDIT_TYPE_PARENT);
> if (!n)
> return;
> - audit_copy_inode(n, NULL, parent);
> + audit_copy_inode(n, NULL, parent, 0);
> }
>
> if (!found_child) {
> @@ -1971,7 +1971,7 @@ void __audit_inode_child(struct inode *parent,
> }
>
> if (inode)
> - audit_copy_inode(found_child, dentry, inode);
> + audit_copy_inode(found_child, dentry, inode, 0);
> else
> found_child->ino = AUDIT_INO_UNSET;
> }
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-02-01 20:42 ` Nathan Chancellor
@ 2019-02-01 21:05 ` Paul Moore
2019-02-01 21:57 ` Richard Guy Briggs
0 siblings, 1 reply; 13+ messages in thread
From: Paul Moore @ 2019-02-01 21:05 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Richard Guy Briggs, linux-fsdevel, viro, LKML,
Linux-Audit Mailing List, Steve Grubb, Eric Paris
On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > it waits for the missing resource to (possibly never) re-appear.
> >
> > Note the comment above user_path_mountpoint_at():
> > * A umount is a special case for path walking. We're not actually interested
> > * in the inode in this situation, and ESTALE errors can be a problem. We
> > * simply want track down the dentry and vfsmount attached at the mountpoint
> > * and avoid revalidating the last component.
> >
> > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> >
> > Please see the github issue tracker
> > https://github.com/linux-audit/audit-kernel/issues/100
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > fs/namei.c | 2 +-
> > fs/namespace.c | 2 ++
> > include/linux/audit.h | 15 ++++++++++-----
> > include/linux/namei.h | 3 +++
> > kernel/audit.c | 10 +++++++++-
> > kernel/audit.h | 2 +-
> > kernel/auditsc.c | 6 +++---
> > 7 files changed, 29 insertions(+), 11 deletions(-)
...
> > /* Copy inode data into an audit_names. */
> > void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > - struct inode *inode)
> > + struct inode *inode, unsigned int flags)
> > {
> > name->ino = inode->i_ino;
> > name->dev = inode->i_sb->s_dev;
> > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
>
> I don't know if this has been reported or if I am missing something but
> on next-20190201, this line causes an error with arm allyesconfig (as
> CONFIG_AUDITSYCALL doesn't get selected):
...
> CC kernel/audit.o
> kernel/audit.c: In function 'audit_copy_inode':
> kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in this function); did you mean 'AUDIT_TYPE_NORMAL'?
> if (flags & AUDIT_INODE_NOEVAL) {
> ^~~~~~~~~~~~~~~~~~
> AUDIT_TYPE_NORMAL
> kernel/audit.c:2130:14: note: each undeclared identifier is reported only once for each function it appears in
> make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> make: *** [Makefile:296: __build_one_by_one] Error 2
I hadn't seen this reported to the audit list yet, thanks for letting us now.
Richard, please submit a patch to fix this ASAP. Looking at this, the
obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
sure if that itself is going to cause problems (it doesn't look like
it). Actually, thinking out loud, I wonder if we shouldn't move
audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
audit_log_name() too?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-02-01 21:05 ` Paul Moore
@ 2019-02-01 21:57 ` Richard Guy Briggs
2019-02-01 22:27 ` Paul Moore
2019-02-01 22:49 ` Richard Guy Briggs
0 siblings, 2 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2019-02-01 21:57 UTC (permalink / raw)
To: Paul Moore
Cc: Nathan Chancellor, linux-fsdevel, viro, LKML,
Linux-Audit Mailing List, Steve Grubb, Eric Paris
On 2019-02-01 16:05, Paul Moore wrote:
> On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > it waits for the missing resource to (possibly never) re-appear.
> > >
> > > Note the comment above user_path_mountpoint_at():
> > > * A umount is a special case for path walking. We're not actually interested
> > > * in the inode in this situation, and ESTALE errors can be a problem. We
> > > * simply want track down the dentry and vfsmount attached at the mountpoint
> > > * and avoid revalidating the last component.
> > >
> > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > >
> > > Please see the github issue tracker
> > > https://github.com/linux-audit/audit-kernel/issues/100
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > fs/namei.c | 2 +-
> > > fs/namespace.c | 2 ++
> > > include/linux/audit.h | 15 ++++++++++-----
> > > include/linux/namei.h | 3 +++
> > > kernel/audit.c | 10 +++++++++-
> > > kernel/audit.h | 2 +-
> > > kernel/auditsc.c | 6 +++---
> > > 7 files changed, 29 insertions(+), 11 deletions(-)
>
> ...
>
> > > /* Copy inode data into an audit_names. */
> > > void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > > - struct inode *inode)
> > > + struct inode *inode, unsigned int flags)
> > > {
> > > name->ino = inode->i_ino;
> > > name->dev = inode->i_sb->s_dev;
> > > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> >
> > I don't know if this has been reported or if I am missing something but
> > on next-20190201, this line causes an error with arm allyesconfig (as
> > CONFIG_AUDITSYCALL doesn't get selected):
>
> ...
>
> > CC kernel/audit.o
> > kernel/audit.c: In function 'audit_copy_inode':
> > kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in this function); did you mean 'AUDIT_TYPE_NORMAL'?
> > if (flags & AUDIT_INODE_NOEVAL) {
> > ^~~~~~~~~~~~~~~~~~
> > AUDIT_TYPE_NORMAL
> > kernel/audit.c:2130:14: note: each undeclared identifier is reported only once for each function it appears in
> > make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> > make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> > make: *** [Makefile:296: __build_one_by_one] Error 2
>
> I hadn't seen this reported to the audit list yet, thanks for letting us now.
Thanks Nathan for the report.
> Richard, please submit a patch to fix this ASAP. Looking at this, the
> obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
> sure if that itself is going to cause problems (it doesn't look like
> it). Actually, thinking out loud, I wonder if we shouldn't move
> audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
> audit_log_name() too?
They have all been moved in ghak105 v2 patch 2 which is in your queue.
Lemme think if there is a quicker simpler solution for this patch.
That ghak105 v2.patch2 will have a number of merge conflicts and I've
already sorted those out, so I'm willing to post a respin (v3) of it to
make your life easier. (audit_log_key() should also be moved, now that
I check...)
> 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] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-02-01 21:57 ` Richard Guy Briggs
@ 2019-02-01 22:27 ` Paul Moore
2019-02-01 22:49 ` Richard Guy Briggs
1 sibling, 0 replies; 13+ messages in thread
From: Paul Moore @ 2019-02-01 22:27 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Nathan Chancellor, linux-fsdevel, viro, LKML,
Linux-Audit Mailing List, Steve Grubb, Eric Paris
On Fri, Feb 1, 2019 at 4:57 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-02-01 16:05, Paul Moore wrote:
> > On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > > On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > > it waits for the missing resource to (possibly never) re-appear.
> > > >
> > > > Note the comment above user_path_mountpoint_at():
> > > > * A umount is a special case for path walking. We're not actually interested
> > > > * in the inode in this situation, and ESTALE errors can be a problem. We
> > > > * simply want track down the dentry and vfsmount attached at the mountpoint
> > > > * and avoid revalidating the last component.
> > > >
> > > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > > >
> > > > Please see the github issue tracker
> > > > https://github.com/linux-audit/audit-kernel/issues/100
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > fs/namei.c | 2 +-
> > > > fs/namespace.c | 2 ++
> > > > include/linux/audit.h | 15 ++++++++++-----
> > > > include/linux/namei.h | 3 +++
> > > > kernel/audit.c | 10 +++++++++-
> > > > kernel/audit.h | 2 +-
> > > > kernel/auditsc.c | 6 +++---
> > > > 7 files changed, 29 insertions(+), 11 deletions(-)
> >
> > ...
> >
> > > > /* Copy inode data into an audit_names. */
> > > > void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > > > - struct inode *inode)
> > > > + struct inode *inode, unsigned int flags)
> > > > {
> > > > name->ino = inode->i_ino;
> > > > name->dev = inode->i_sb->s_dev;
> > > > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> > >
> > > I don't know if this has been reported or if I am missing something but
> > > on next-20190201, this line causes an error with arm allyesconfig (as
> > > CONFIG_AUDITSYCALL doesn't get selected):
> >
> > ...
> >
> > > CC kernel/audit.o
> > > kernel/audit.c: In function 'audit_copy_inode':
> > > kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in this function); did you mean 'AUDIT_TYPE_NORMAL'?
> > > if (flags & AUDIT_INODE_NOEVAL) {
> > > ^~~~~~~~~~~~~~~~~~
> > > AUDIT_TYPE_NORMAL
> > > kernel/audit.c:2130:14: note: each undeclared identifier is reported only once for each function it appears in
> > > make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> > > make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> > > make: *** [Makefile:296: __build_one_by_one] Error 2
> >
> > I hadn't seen this reported to the audit list yet, thanks for letting us now.
>
> Thanks Nathan for the report.
>
> > Richard, please submit a patch to fix this ASAP. Looking at this, the
> > obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
> > sure if that itself is going to cause problems (it doesn't look like
> > it). Actually, thinking out loud, I wonder if we shouldn't move
> > audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
> > audit_log_name() too?
>
> They have all been moved in ghak105 v2 patch 2 which is in your queue.
> Lemme think if there is a quicker simpler solution for this patch ...
I think there was some confusion about the patch; see the other
thread. If you make that small requested change I'll merge it and I
think we can call this resolved.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
2019-02-01 21:57 ` Richard Guy Briggs
2019-02-01 22:27 ` Paul Moore
@ 2019-02-01 22:49 ` Richard Guy Briggs
1 sibling, 0 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2019-02-01 22:49 UTC (permalink / raw)
To: Paul Moore
Cc: Nathan Chancellor, linux-fsdevel, viro, LKML,
Linux-Audit Mailing List, Steve Grubb, Eric Paris
On 2019-02-01 16:57, Richard Guy Briggs wrote:
> On 2019-02-01 16:05, Paul Moore wrote:
> > On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > > On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > > it waits for the missing resource to (possibly never) re-appear.
> > > >
> > > > Note the comment above user_path_mountpoint_at():
> > > > * A umount is a special case for path walking. We're not actually interested
> > > > * in the inode in this situation, and ESTALE errors can be a problem. We
> > > > * simply want track down the dentry and vfsmount attached at the mountpoint
> > > > * and avoid revalidating the last component.
> > > >
> > > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > > >
> > > > Please see the github issue tracker
> > > > https://github.com/linux-audit/audit-kernel/issues/100
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > fs/namei.c | 2 +-
> > > > fs/namespace.c | 2 ++
> > > > include/linux/audit.h | 15 ++++++++++-----
> > > > include/linux/namei.h | 3 +++
> > > > kernel/audit.c | 10 +++++++++-
> > > > kernel/audit.h | 2 +-
> > > > kernel/auditsc.c | 6 +++---
> > > > 7 files changed, 29 insertions(+), 11 deletions(-)
> >
> > ...
> >
> > > > /* Copy inode data into an audit_names. */
> > > > void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > > > - struct inode *inode)
> > > > + struct inode *inode, unsigned int flags)
> > > > {
> > > > name->ino = inode->i_ino;
> > > > name->dev = inode->i_sb->s_dev;
> > > > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> > >
> > > I don't know if this has been reported or if I am missing something but
> > > on next-20190201, this line causes an error with arm allyesconfig (as
> > > CONFIG_AUDITSYCALL doesn't get selected):
> >
> > ...
> >
> > > CC kernel/audit.o
> > > kernel/audit.c: In function 'audit_copy_inode':
> > > kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in this function); did you mean 'AUDIT_TYPE_NORMAL'?
> > > if (flags & AUDIT_INODE_NOEVAL) {
> > > ^~~~~~~~~~~~~~~~~~
> > > AUDIT_TYPE_NORMAL
> > > kernel/audit.c:2130:14: note: each undeclared identifier is reported only once for each function it appears in
> > > make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> > > make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> > > make: *** [Makefile:296: __build_one_by_one] Error 2
> >
> > I hadn't seen this reported to the audit list yet, thanks for letting us now.
>
> Thanks Nathan for the report.
>
> > Richard, please submit a patch to fix this ASAP. Looking at this, the
> > obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
> > sure if that itself is going to cause problems (it doesn't look like
> > it). Actually, thinking out loud, I wonder if we shouldn't move
> > audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
> > audit_log_name() too?
>
> They have all been moved in ghak105 v2 patch 2 which is in your queue.
> Lemme think if there is a quicker simpler solution for this patch.
> That ghak105 v2.patch2 will have a number of merge conflicts and I've
> already sorted those out, so I'm willing to post a respin (v3) of it to
> make your life easier. (audit_log_key() should also be moved, now that
> I check...)
The really simple quick and dirty fix, which is cheating, is to move
that AUDIT_INODE_NOEVAL definition 15 lines up in include/linux/audit.h
just above the #ifdef CONFIG_AUDITSYSCALL.
The right fix is in ghak105 v2.patch2 with the audit_context pointer
still in the audit_buffer. Lemme rebase, fix that and repost...
> > paul moore
>
> - RGB
- 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] 13+ messages in thread
end of thread, other threads:[~2019-02-01 22:49 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-23 18:34 [PATCH ghak100 V2 0/2] audit: avoid umount hangs on missing mount Richard Guy Briggs
2019-01-23 18:34 ` [PATCH ghak100 V2 1/2] audit: more filter PATH records keyed on filesystem magic Richard Guy Briggs
2019-01-25 21:36 ` Paul Moore
2019-01-23 18:35 ` [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount Richard Guy Briggs
2019-01-25 21:45 ` Paul Moore
2019-01-25 22:27 ` Richard Guy Briggs
2019-01-28 23:25 ` Paul Moore
2019-01-31 2:20 ` Paul Moore
2019-02-01 20:42 ` Nathan Chancellor
2019-02-01 21:05 ` Paul Moore
2019-02-01 21:57 ` Richard Guy Briggs
2019-02-01 22:27 ` Paul Moore
2019-02-01 22:49 ` Richard Guy Briggs
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).