From: Davidlohr Bueso <[email protected]>
This series is the first in a few where I'm planning on removing
the mmap_sem need for exe_file serialization. This is absurd and
needs to have its own locking. Anyway, this is the final goal, and
this series is just the first of a few that deals with unifying users
of exe_file.
For now we only deal with audit and tomoyo, the most obvious naughty
users, which only take the mmap_sem for exe_file. Over the years,
relying on the mmap_sem for exe_file has made some callers increasingly
messy and it is not as straightforward.
Essentially, we want to convert:
down_read(&mm->mmap_sem);
do_something_with(mm->exe_file);
up_read(&mm->mmap_sem);
to:
exe_file = get_mm_exe_file(mm); <--- mmap_sem is only held here.
do_something_with(mm->exe_file);
fput(exe_file);
On its own, these patches already have value in that we reduce mmap_sem hold
times and critical region. Once all users are standardized, converting the
lock rules will be much easier.
Thanks!
Davidlohr Bueso (3):
kernel/audit: consolidate handling of mm->exe_file
kernel/audit: robustify handling of mm->exe_file
security/tomoyo: robustify handling of mm->exe_file
kernel/audit.c | 9 +--------
kernel/audit.h | 20 ++++++++++++++++++++
kernel/auditsc.c | 9 +--------
security/tomoyo/common.c | 41 ++++++++++++++++++++++++++++++++++++++---
security/tomoyo/common.h | 1 -
security/tomoyo/util.c | 22 ----------------------
6 files changed, 60 insertions(+), 42 deletions(-)
--
2.1.4
From: Davidlohr Bueso <[email protected]>
This patch adds a audit_log_d_path_exe() helper function
to share how we handle auditing of the exe_file's path.
Used by both audit and auditsc. No functionality is changed.
Cc: Paul Moore <[email protected]>
Cc: Eric Paris <[email protected]>
Cc: [email protected]
Signed-off-by: Davidlohr Bueso <[email protected]>
---
Compile tested only.
kernel/audit.c | 9 +--------
kernel/audit.h | 14 ++++++++++++++
kernel/auditsc.c | 9 +--------
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 72ab759..9b49f76 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1842,7 +1842,6 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
{
const struct cred *cred;
char comm[sizeof(tsk->comm)];
- struct mm_struct *mm = tsk->mm;
char *tty;
if (!ab)
@@ -1878,13 +1877,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
audit_log_format(ab, " comm=");
audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
- if (mm) {
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- } else
- audit_log_format(ab, " exe=(null)");
+ audit_log_d_path_exe(ab, tsk->mm);
audit_log_task_context(ab);
}
EXPORT_SYMBOL(audit_log_task_info);
diff --git a/kernel/audit.h b/kernel/audit.h
index 1caa0d3..510901f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -257,6 +257,20 @@ extern struct list_head audit_filter_list[];
extern struct audit_entry *audit_dupe_rule(struct audit_krule *old);
+static inline void audit_log_d_path_exe(struct audit_buffer *ab,
+ struct mm_struct *mm)
+{
+ if (!mm) {
+ audit_log_format(ab, " exe=(null)");
+ return;
+ }
+
+ down_read(&mm->mmap_sem);
+ if (mm->exe_file)
+ audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
+ up_read(&mm->mmap_sem);
+}
+
/* audit watch functions */
#ifdef CONFIG_AUDIT_WATCH
extern void audit_put_watch(struct audit_watch *watch);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index dc4ae70..84c74d0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2361,7 +2361,6 @@ static void audit_log_task(struct audit_buffer *ab)
kuid_t auid, uid;
kgid_t gid;
unsigned int sessionid;
- struct mm_struct *mm = current->mm;
char comm[sizeof(current->comm)];
auid = audit_get_loginuid(current);
@@ -2376,13 +2375,7 @@ static void audit_log_task(struct audit_buffer *ab)
audit_log_task_context(ab);
audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
audit_log_untrustedstring(ab, get_task_comm(comm, current));
- if (mm) {
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- } else
- audit_log_format(ab, " exe=(null)");
+ audit_log_d_path_exe(ab, current->mm);
}
/**
--
2.1.4
From: Davidlohr Bueso <[email protected]>
The mm->exe_file is currently serialized with mmap_sem (shared)
in order to both safely (1) read the file and (2) audit it via
audit_log_d_path(). Good users will, on the other hand, make use
of the more standard get_mm_exe_file(), requiring only holding
the mmap_sem to read the value, and relying on reference counting
to make sure that the exe file won't dissapear underneath us.
This is safe as audit_log_d_path() does not need the mmap_sem --
...and if it did we seriously need to fix that.
Additionally, upon NULL return of get_mm_exe_file, we also call
audit_log_format(ab, " exe=(null)").
Cc: Paul Moore <[email protected]>
Cc: Eric Paris <[email protected]>
Cc: [email protected]
Signed-off-by: Davidlohr Bueso <[email protected]>
---
Compiled tested only.
kernel/audit.h | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index 510901f..17020f0 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -20,6 +20,7 @@
*/
#include <linux/fs.h>
+#include <linux/file.h>
#include <linux/audit.h>
#include <linux/skbuff.h>
#include <uapi/linux/mqueue.h>
@@ -260,15 +261,20 @@ extern struct audit_entry *audit_dupe_rule(struct audit_krule *old);
static inline void audit_log_d_path_exe(struct audit_buffer *ab,
struct mm_struct *mm)
{
- if (!mm) {
- audit_log_format(ab, " exe=(null)");
- return;
- }
-
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
+ struct file *exe_file;
+
+ if (!mm)
+ goto out_null;
+
+ exe_file = get_mm_exe_file(mm);
+ if (!exe_file)
+ goto out_null;
+
+ audit_log_d_path(ab, " exe=", &exe_file->f_path);
+ fput(exe_file);
+ return;
+out_null:
+ audit_log_format(ab, " exe=(null)");
}
/* audit watch functions */
--
2.1.4
From: Davidlohr Bueso <[email protected]>
The mm->exe_file is currently serialized with mmap_sem (shared)
in order to both safely (1) read the file and (2) compute the
realpath by calling tomoyo_realpath_from_path, making it an absolute
overkill. Good users will, on the other hand, make use of the more
standard get_mm_exe_file(), requiring only holding the mmap_sem to
read the value, and relying on reference counting to make sure that
the exe file won't dissappear underneath us.
When returning from tomoyo_get_exe, we'll hold reference to the exe's
f_path, make sure we put it back when done at the end of the
manager call. This patch also does some cleanups around the function,
such as moving it into common.c and changing the args.
Cc: Kentaro TKentaro Takeda <[email protected]>
Cc: Tetsuo Handa <[email protected]>
Cc: [email protected]
Signed-off-by: Davidlohr Bueso <[email protected]>
---
Compile tested only.
security/tomoyo/common.c | 41 ++++++++++++++++++++++++++++++++++++++---
security/tomoyo/common.h | 1 -
security/tomoyo/util.c | 22 ----------------------
3 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index e0fb750..f55129f 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -908,6 +908,33 @@ static void tomoyo_read_manager(struct tomoyo_io_buffer *head)
}
/**
+ * tomoyo_get_exe - Get tomoyo_realpath() of current process.
+ *
+ * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
+ *
+ * A successful return will leave the caller with two responsibilities when done
+ * handling the realpath:
+ * (1) path_put the exe_file's path refcount.
+ * (2) kfree return buffer.
+ */
+static const char *tomoyo_get_exe(struct mm_struct *mm)
+{
+ struct file *exe_file;
+ const char *cp = NULL;
+
+ if (!mm)
+ return NULL;
+ exe_file = get_mm_exe_file(mm);
+ if (!exe_file)
+ return NULL;
+
+ cp = tomoyo_realpath_from_path(&exe_file->f_path);
+ path_get(&exe_file->f_path);
+ fput(exe_file);
+ return cp;
+}
+
+/**
* tomoyo_manager - Check whether the current process is a policy manager.
*
* Returns true if the current process is permitted to modify policy
@@ -920,20 +947,26 @@ static bool tomoyo_manager(void)
struct tomoyo_manager *ptr;
const char *exe;
const struct task_struct *task = current;
- const struct tomoyo_path_info *domainname = tomoyo_domain()->domainname;
+ struct mm_struct *mm = current->mm;
+ const struct tomoyo_path_info *domainname;
bool found = false;
+ domainname = tomoyo_domain()->domainname;
+
if (!tomoyo_policy_loaded)
return true;
if (!tomoyo_manage_by_non_root &&
(!uid_eq(task->cred->uid, GLOBAL_ROOT_UID) ||
!uid_eq(task->cred->euid, GLOBAL_ROOT_UID)))
return false;
- exe = tomoyo_get_exe();
+
+ exe = tomoyo_get_exe(mm);
if (!exe)
return false;
+
list_for_each_entry_rcu(ptr, &tomoyo_kernel_namespace.
- policy_list[TOMOYO_ID_MANAGER], head.list) {
+ policy_list[TOMOYO_ID_MANAGER],
+ head.list) {
if (!ptr->head.is_deleted &&
(!tomoyo_pathcmp(domainname, ptr->manager) ||
!strcmp(exe, ptr->manager->name))) {
@@ -950,6 +983,8 @@ static bool tomoyo_manager(void)
last_pid = pid;
}
}
+
+ path_put(&mm->exe_file->f_path);
kfree(exe);
return found;
}
diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index b897d48..fc89eba 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -947,7 +947,6 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
char *tomoyo_read_token(struct tomoyo_acl_param *param);
char *tomoyo_realpath_from_path(struct path *path);
char *tomoyo_realpath_nofollow(const char *pathname);
-const char *tomoyo_get_exe(void);
const char *tomoyo_yesno(const unsigned int value);
const struct tomoyo_path_info *tomoyo_compare_name_union
(const struct tomoyo_path_info *name, const struct tomoyo_name_union *ptr);
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 2952ba5..7eff479 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -939,28 +939,6 @@ bool tomoyo_path_matches_pattern(const struct tomoyo_path_info *filename,
}
/**
- * tomoyo_get_exe - Get tomoyo_realpath() of current process.
- *
- * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
- *
- * This function uses kzalloc(), so the caller must call kfree()
- * if this function didn't return NULL.
- */
-const char *tomoyo_get_exe(void)
-{
- struct mm_struct *mm = current->mm;
- const char *cp = NULL;
-
- if (!mm)
- return NULL;
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- cp = tomoyo_realpath_from_path(&mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- return cp;
-}
-
-/**
* tomoyo_get_mode - Get MAC mode.
*
* @ns: Pointer to "struct tomoyo_policy_namespace".
--
2.1.4
On Wed, Feb 18, 2015 at 7:10 PM, Davidlohr Bueso <[email protected]> wrote:
> From: Davidlohr Bueso <[email protected]>
>
> This patch adds a audit_log_d_path_exe() helper function
> to share how we handle auditing of the exe_file's path.
> Used by both audit and auditsc. No functionality is changed.
>
> Cc: Paul Moore <[email protected]>
> Cc: Eric Paris <[email protected]>
> Cc: [email protected]
> Signed-off-by: Davidlohr Bueso <[email protected]>
> ---
>
> Compile tested only.
>
> kernel/audit.c | 9 +--------
> kernel/audit.h | 14 ++++++++++++++
> kernel/auditsc.c | 9 +--------
> 3 files changed, 16 insertions(+), 16 deletions(-)
I'd prefer if the audit_log_d_path_exe() helper wasn't a static inline.
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -257,6 +257,20 @@ extern struct list_head audit_filter_list[];
>
> extern struct audit_entry *audit_dupe_rule(struct audit_krule *old);
>
> +static inline void audit_log_d_path_exe(struct audit_buffer *ab,
> + struct mm_struct *mm)
> +{
> + if (!mm) {
> + audit_log_format(ab, " exe=(null)");
> + return;
> + }
> +
> + down_read(&mm->mmap_sem);
> + if (mm->exe_file)
> + audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
> + up_read(&mm->mmap_sem);
> +}
> +
> /* audit watch functions */
> #ifdef CONFIG_AUDIT_WATCH
> extern void audit_put_watch(struct audit_watch *watch);
--
paul moore
http://www.paul-moore.com
On Wed, 2015-02-18 at 16:10 -0800, Davidlohr Bueso wrote:
> +static const char *tomoyo_get_exe(struct mm_struct *mm)
> +{
> + struct file *exe_file;
> + const char *cp = NULL;
> +
> + if (!mm)
> + return NULL;
> + exe_file = get_mm_exe_file(mm);
> + if (!exe_file)
> + return NULL;
> +
> + cp = tomoyo_realpath_from_path(&exe_file->f_path);
tomoyo_realpath_from_path can return NULL here, thus we'd leak the
f_path in the caller... I guess this should be:
> + path_get(&exe_file->f_path);
if (cp)
path_get(&exe_file->f_path);
> + fput(exe_file);
> + return cp;
> +}
Thank you, but I think this patch is wrong and redundant.
Davidlohr Bueso wrote:
> On Wed, 2015-02-18 at 16:10 -0800, Davidlohr Bueso wrote:
> > +static const char *tomoyo_get_exe(struct mm_struct *mm)
> > +{
> > + struct file *exe_file;
> > + const char *cp = NULL;
> > +
> > + if (!mm)
> > + return NULL;
> > + exe_file = get_mm_exe_file(mm);
> > + if (!exe_file)
> > + return NULL;
> > +
> > + cp = tomoyo_realpath_from_path(&exe_file->f_path);
>
> tomoyo_realpath_from_path can return NULL here, thus we'd leak the
> f_path in the caller... I guess this should be:
>
> > + path_get(&exe_file->f_path);
>
> if (cp)
> path_get(&exe_file->f_path);
>
Why do we need to let the caller call path_put() ?
There is no need to do like proc_exe_link() does, for
tomoyo_get_exe() returns pathname as "char *".
> > + fput(exe_file);
> > + return cp;
> > +}
On Thu, 2015-02-19 at 20:07 +0900, Tetsuo Handa wrote:
> Why do we need to let the caller call path_put() ?
> There is no need to do like proc_exe_link() does, for
> tomoyo_get_exe() returns pathname as "char *".
Having the pathname doesn't guarantee anything later, and thus doesn't
seem very robust in the manager call if it can be dropped during the
call... or can this never occur in this context?
Davidlohr Bueso wrote:
> On Thu, 2015-02-19 at 20:07 +0900, Tetsuo Handa wrote:
> > Why do we need to let the caller call path_put() ?
> > There is no need to do like proc_exe_link() does, for
> > tomoyo_get_exe() returns pathname as "char *".
>
> Having the pathname doesn't guarantee anything later, and thus doesn't
> seem very robust in the manager call if it can be dropped during the
> call... or can this never occur in this context?
>
tomoyo_get_exe() returns the pathname of executable of current thread.
The executable of current thread cannot be changed while current thread
is inside the manager call. Although the pathname of executable of
current thread could be changed by other threads via namespace manipulation
like pivot_root(), holding a reference guarantees nothing. Your patch helps
for avoiding memory allocation with mmap_sem held, but does not robustify
handling of mm->exe_file for tomoyo.
On Fri, 2015-02-20 at 07:11 +0900, Tetsuo Handa wrote:
> Davidlohr Bueso wrote:
> > On Thu, 2015-02-19 at 20:07 +0900, Tetsuo Handa wrote:
> > > Why do we need to let the caller call path_put() ?
> > > There is no need to do like proc_exe_link() does, for
> > > tomoyo_get_exe() returns pathname as "char *".
> >
> > Having the pathname doesn't guarantee anything later, and thus doesn't
> > seem very robust in the manager call if it can be dropped during the
> > call... or can this never occur in this context?
> >
> tomoyo_get_exe() returns the pathname of executable of current thread.
> The executable of current thread cannot be changed while current thread
> is inside the manager call. Although the pathname of executable of
> current thread could be changed by other threads via namespace manipulation
> like pivot_root(), holding a reference guarantees nothing. Your patch helps
> for avoiding memory allocation with mmap_sem held, but does not robustify
> handling of mm->exe_file for tomoyo.
Fair enough, I won't argue. This is beyond the scope if what I'm trying
to accomplish here anyway. Are you ok with this instead?
8<--------------------------------------------------------------------
Subject: [PATCH v2 3/3] tomoyo: reduce mmap_sem hold for mm->exe_file
The mm->exe_file is currently serialized with mmap_sem (shared) in order
to both safely (1) read the file and (2) compute the realpath by calling
tomoyo_realpath_from_path, making it an absolute overkill. Good users will,
on the other hand, make use of the more standard get_mm_exe_file(), requiring
only holding the mmap_sem to read the value, and relying on reference
counting to make sure that the exe_file won't disappear underneath us.
While at it, do some very minor cleanups around tomoyo_get_exe(), such as
make it local to common.c
Signed-off-by: Davidlohr Bueso <[email protected]>
---
security/tomoyo/common.c | 33 +++++++++++++++++++++++++++++++--
security/tomoyo/common.h | 1 -
security/tomoyo/util.c | 22 ----------------------
3 files changed, 31 insertions(+), 25 deletions(-)
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index e0fb750..73ce629 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -908,6 +908,31 @@ static void tomoyo_read_manager(struct tomoyo_io_buffer *head)
}
/**
+ * tomoyo_get_exe - Get tomoyo_realpath() of current process.
+ *
+ * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
+ *
+ * This function uses kzalloc(), so the caller must call kfree()
+ * if this function didn't return NULL.
+ */
+static const char *tomoyo_get_exe(void)
+{
+ struct file *exe_file;
+ const char *cp = NULL;
+ struct mm_struct *mm = current->mm;
+
+ if (!mm)
+ return NULL;
+ exe_file = get_mm_exe_file(mm);
+ if (!exe_file)
+ return NULL;
+
+ cp = tomoyo_realpath_from_path(&exe_file->f_path);
+ fput(exe_file);
+ return cp;
+}
+
+/**
* tomoyo_manager - Check whether the current process is a policy manager.
*
* Returns true if the current process is permitted to modify policy
@@ -920,9 +945,11 @@ static bool tomoyo_manager(void)
struct tomoyo_manager *ptr;
const char *exe;
const struct task_struct *task = current;
- const struct tomoyo_path_info *domainname = tomoyo_domain()->domainname;
+ const struct tomoyo_path_info *domainname;
bool found = false;
+ domainname = tomoyo_domain()->domainname;
+
if (!tomoyo_policy_loaded)
return true;
if (!tomoyo_manage_by_non_root &&
@@ -932,8 +959,10 @@ static bool tomoyo_manager(void)
exe = tomoyo_get_exe();
if (!exe)
return false;
+
list_for_each_entry_rcu(ptr, &tomoyo_kernel_namespace.
- policy_list[TOMOYO_ID_MANAGER], head.list) {
+ policy_list[TOMOYO_ID_MANAGER],
+ head.list) {
if (!ptr->head.is_deleted &&
(!tomoyo_pathcmp(domainname, ptr->manager) ||
!strcmp(exe, ptr->manager->name))) {
diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index b897d48..fc89eba 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -947,7 +947,6 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
char *tomoyo_read_token(struct tomoyo_acl_param *param);
char *tomoyo_realpath_from_path(struct path *path);
char *tomoyo_realpath_nofollow(const char *pathname);
-const char *tomoyo_get_exe(void);
const char *tomoyo_yesno(const unsigned int value);
const struct tomoyo_path_info *tomoyo_compare_name_union
(const struct tomoyo_path_info *name, const struct tomoyo_name_union *ptr);
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 2952ba5..7eff479 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -939,28 +939,6 @@ bool tomoyo_path_matches_pattern(const struct tomoyo_path_info *filename,
}
/**
- * tomoyo_get_exe - Get tomoyo_realpath() of current process.
- *
- * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
- *
- * This function uses kzalloc(), so the caller must call kfree()
- * if this function didn't return NULL.
- */
-const char *tomoyo_get_exe(void)
-{
- struct mm_struct *mm = current->mm;
- const char *cp = NULL;
-
- if (!mm)
- return NULL;
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- cp = tomoyo_realpath_from_path(&mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- return cp;
-}
-
-/**
* tomoyo_get_mode - Get MAC mode.
*
* @ns: Pointer to "struct tomoyo_policy_namespace".
--
2.1.4
On Wed, 2015-02-18 at 22:23 -0500, Paul Moore wrote:
> I'd prefer if the audit_log_d_path_exe() helper wasn't a static inline.
What do you have in mind? At least in code size static inlining wins:
text data bss dec hex filename
14423 284 676 15383 3c17 kernel/audit.o
14407 284 676 15367 3c07 kernel/audit.o-thispatch
14474 284 676 15434 3c4a kernel/audit.o-noninline
Thanks,
Davidlohr
On Fri, Feb 20, 2015 at 8:23 PM, Davidlohr Bueso <[email protected]> wrote:
> On Wed, 2015-02-18 at 22:23 -0500, Paul Moore wrote:
>> I'd prefer if the audit_log_d_path_exe() helper wasn't a static inline.
>
> What do you have in mind?
Pretty much what I said before, audit_log_d_path_exe() as a
traditional function and not an inline. Put the function in
kernel/audit.c.
--
paul moore
http://www.paul-moore.com
On Sat, 2015-02-21 at 08:45 -0500, Paul Moore wrote:
> On Fri, Feb 20, 2015 at 8:23 PM, Davidlohr Bueso <[email protected]> wrote:
> > On Wed, 2015-02-18 at 22:23 -0500, Paul Moore wrote:
> >> I'd prefer if the audit_log_d_path_exe() helper wasn't a static inline.
> >
> > What do you have in mind?
>
> Pretty much what I said before, audit_log_d_path_exe() as a
> traditional function and not an inline. Put the function in
> kernel/audit.c.
well yes I know that, which is why I showed you the code sizes. Now
again, do you have any reason? This function will only get less bulky in
the future.
On Sat, Feb 21, 2015 at 10:00 AM, Davidlohr Bueso <[email protected]> wrote:
> On Sat, 2015-02-21 at 08:45 -0500, Paul Moore wrote:
>> On Fri, Feb 20, 2015 at 8:23 PM, Davidlohr Bueso <[email protected]> wrote:
>> > On Wed, 2015-02-18 at 22:23 -0500, Paul Moore wrote:
>> >> I'd prefer if the audit_log_d_path_exe() helper wasn't a static inline.
>> >
>> > What do you have in mind?
>>
>> Pretty much what I said before, audit_log_d_path_exe() as a
>> traditional function and not an inline. Put the function in
>> kernel/audit.c.
>
> well yes I know that, which is why I showed you the code sizes. Now
> again, do you have any reason? This function will only get less bulky in
> the future.
The code size was pretty negligible from my point of view, not enough
to outweigh my preference for a non-inlined version of the function.
Also, I expect this function will be one of the things that gets
shuffled/reworked in the coming months as we make some architectural
changes to audit.
--
paul moore
http://www.paul-moore.com
This patch adds a audit_log_d_path_exe() helper function
to share how we handle auditing of the exe_file's path.
Used by both audit and auditsc. No functionality is changed.
Signed-off-by: Davidlohr Bueso <[email protected]>
---
changes from v1: created normal function for helper.
kernel/audit.c | 23 +++++++++++++++--------
kernel/audit.h | 3 +++
kernel/auditsc.c | 9 +--------
3 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 72ab759..a71cbfe 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1838,11 +1838,24 @@ error_path:
}
EXPORT_SYMBOL(audit_log_task_context);
+void audit_log_d_path_exe(struct audit_buffer *ab,
+ struct mm_struct *mm)
+{
+ if (!mm) {
+ audit_log_format(ab, " exe=(null)");
+ return;
+ }
+
+ down_read(&mm->mmap_sem);
+ if (mm->exe_file)
+ audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
+ up_read(&mm->mmap_sem);
+}
+
void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
{
const struct cred *cred;
char comm[sizeof(tsk->comm)];
- struct mm_struct *mm = tsk->mm;
char *tty;
if (!ab)
@@ -1878,13 +1891,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
audit_log_format(ab, " comm=");
audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
- if (mm) {
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- } else
- audit_log_format(ab, " exe=(null)");
+ audit_log_d_path_exe(ab, tsk->mm);
audit_log_task_context(ab);
}
EXPORT_SYMBOL(audit_log_task_info);
diff --git a/kernel/audit.h b/kernel/audit.h
index 1caa0d3..d641f9b 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -257,6 +257,9 @@ extern struct list_head audit_filter_list[];
extern struct audit_entry *audit_dupe_rule(struct audit_krule *old);
+extern void audit_log_d_path_exe(struct audit_buffer *ab,
+ struct mm_struct *mm);
+
/* audit watch functions */
#ifdef CONFIG_AUDIT_WATCH
extern void audit_put_watch(struct audit_watch *watch);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index dc4ae70..84c74d0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2361,7 +2361,6 @@ static void audit_log_task(struct audit_buffer *ab)
kuid_t auid, uid;
kgid_t gid;
unsigned int sessionid;
- struct mm_struct *mm = current->mm;
char comm[sizeof(current->comm)];
auid = audit_get_loginuid(current);
@@ -2376,13 +2375,7 @@ static void audit_log_task(struct audit_buffer *ab)
audit_log_task_context(ab);
audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
audit_log_untrustedstring(ab, get_task_comm(comm, current));
- if (mm) {
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- } else
- audit_log_format(ab, " exe=(null)");
+ audit_log_d_path_exe(ab, current->mm);
}
/**
--
2.1.4
The mm->exe_file is currently serialized with mmap_sem (shared)
in order to both safely (1) read the file and (2) audit it via
audit_log_d_path(). Good users will, on the other hand, make use
of the more standard get_mm_exe_file(), requiring only holding
the mmap_sem to read the value, and relying on reference counting
to make sure that the exe file won't dissapear underneath us.
Additionally, upon NULL return of get_mm_exe_file, we also call
audit_log_format(ab, " exe=(null)").
Signed-off-by: Davidlohr Bueso <[email protected]>
---
changes from v1: rebased on top of 1/1.
kernel/audit.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index a71cbfe..b446d54 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -43,6 +43,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/file.h>
#include <linux/init.h>
#include <linux/types.h>
#include <linux/atomic.h>
@@ -1841,15 +1842,20 @@ EXPORT_SYMBOL(audit_log_task_context);
void audit_log_d_path_exe(struct audit_buffer *ab,
struct mm_struct *mm)
{
- if (!mm) {
- audit_log_format(ab, " exe=(null)");
- return;
- }
+ struct file *exe_file;
+
+ if (!mm)
+ goto out_null;
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
+ exe_file = get_mm_exe_file(mm);
+ if (!exe_file)
+ goto out_null;
+
+ audit_log_d_path(ab, " exe=", &exe_file->f_path);
+ fput(exe_file);
+ return;
+out_null:
+ audit_log_format(ab, " exe=(null)");
}
void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
--
2.1.4
On Sunday, February 22, 2015 06:20:00 PM Davidlohr Bueso wrote:
> This patch adds a audit_log_d_path_exe() helper function
> to share how we handle auditing of the exe_file's path.
> Used by both audit and auditsc. No functionality is changed.
>
> Signed-off-by: Davidlohr Bueso <[email protected]>
> ---
>
> changes from v1: created normal function for helper.
>
> kernel/audit.c | 23 +++++++++++++++--------
> kernel/audit.h | 3 +++
> kernel/auditsc.c | 9 +--------
> 3 files changed, 19 insertions(+), 16 deletions(-)
Merged into audit#next.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 72ab759..a71cbfe 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1838,11 +1838,24 @@ error_path:
> }
> EXPORT_SYMBOL(audit_log_task_context);
>
> +void audit_log_d_path_exe(struct audit_buffer *ab,
> + struct mm_struct *mm)
> +{
> + if (!mm) {
> + audit_log_format(ab, " exe=(null)");
> + return;
> + }
> +
> + down_read(&mm->mmap_sem);
> + if (mm->exe_file)
> + audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
> + up_read(&mm->mmap_sem);
> +}
> +
> void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> {
> const struct cred *cred;
> char comm[sizeof(tsk->comm)];
> - struct mm_struct *mm = tsk->mm;
> char *tty;
>
> if (!ab)
> @@ -1878,13 +1891,7 @@ void audit_log_task_info(struct audit_buffer *ab,
> struct task_struct *tsk) audit_log_format(ab, " comm=");
> audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
>
> - if (mm) {
> - down_read(&mm->mmap_sem);
> - if (mm->exe_file)
> - audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
> - up_read(&mm->mmap_sem);
> - } else
> - audit_log_format(ab, " exe=(null)");
> + audit_log_d_path_exe(ab, tsk->mm);
> audit_log_task_context(ab);
> }
> EXPORT_SYMBOL(audit_log_task_info);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 1caa0d3..d641f9b 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -257,6 +257,9 @@ extern struct list_head audit_filter_list[];
>
> extern struct audit_entry *audit_dupe_rule(struct audit_krule *old);
>
> +extern void audit_log_d_path_exe(struct audit_buffer *ab,
> + struct mm_struct *mm);
> +
> /* audit watch functions */
> #ifdef CONFIG_AUDIT_WATCH
> extern void audit_put_watch(struct audit_watch *watch);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index dc4ae70..84c74d0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2361,7 +2361,6 @@ static void audit_log_task(struct audit_buffer *ab)
> kuid_t auid, uid;
> kgid_t gid;
> unsigned int sessionid;
> - struct mm_struct *mm = current->mm;
> char comm[sizeof(current->comm)];
>
> auid = audit_get_loginuid(current);
> @@ -2376,13 +2375,7 @@ static void audit_log_task(struct audit_buffer *ab)
> audit_log_task_context(ab);
> audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> audit_log_untrustedstring(ab, get_task_comm(comm, current));
> - if (mm) {
> - down_read(&mm->mmap_sem);
> - if (mm->exe_file)
> - audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
> - up_read(&mm->mmap_sem);
> - } else
> - audit_log_format(ab, " exe=(null)");
> + audit_log_d_path_exe(ab, current->mm);
> }
>
> /**
--
paul moore
http://www.paul-moore.com
On Sunday, February 22, 2015 06:20:09 PM Davidlohr Bueso wrote:
> The mm->exe_file is currently serialized with mmap_sem (shared)
> in order to both safely (1) read the file and (2) audit it via
> audit_log_d_path(). Good users will, on the other hand, make use
> of the more standard get_mm_exe_file(), requiring only holding
> the mmap_sem to read the value, and relying on reference counting
> to make sure that the exe file won't dissapear underneath us.
>
> Additionally, upon NULL return of get_mm_exe_file, we also call
> audit_log_format(ab, " exe=(null)").
>
> Signed-off-by: Davidlohr Bueso <[email protected]>
> ---
>
> changes from v1: rebased on top of 1/1.
>
> kernel/audit.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
Merged into audit#next.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a71cbfe..b446d54 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -43,6 +43,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/file.h>
> #include <linux/init.h>
> #include <linux/types.h>
> #include <linux/atomic.h>
> @@ -1841,15 +1842,20 @@ EXPORT_SYMBOL(audit_log_task_context);
> void audit_log_d_path_exe(struct audit_buffer *ab,
> struct mm_struct *mm)
> {
> - if (!mm) {
> - audit_log_format(ab, " exe=(null)");
> - return;
> - }
> + struct file *exe_file;
> +
> + if (!mm)
> + goto out_null;
>
> - down_read(&mm->mmap_sem);
> - if (mm->exe_file)
> - audit_log_d_path(ab, " exe=", &mm->exe_file->f_path);
> - up_read(&mm->mmap_sem);
> + exe_file = get_mm_exe_file(mm);
> + if (!exe_file)
> + goto out_null;
> +
> + audit_log_d_path(ab, " exe=", &exe_file->f_path);
> + fput(exe_file);
> + return;
> +out_null:
> + audit_log_format(ab, " exe=(null)");
> }
>
> void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
--
paul moore
http://www.paul-moore.com
On Mon, 2015-02-23 at 16:59 -0500, Paul Moore wrote:
> Merged into audit#next.
hmm Andrew I was hoping you could take these patches. That way we can
easily build on top. Let me know if you think otherwise, as I've got
more ready to send out with a similar email scheme.
Thanks,
Davidlohr
On Mon, Feb 23, 2015 at 5:02 PM, Davidlohr Bueso <[email protected]> wrote:
> On Mon, 2015-02-23 at 16:59 -0500, Paul Moore wrote:
>> Merged into audit#next.
>
> hmm Andrew I was hoping you could take these patches. That way we can
> easily build on top. Let me know if you think otherwise, as I've got
> more ready to send out with a similar email scheme.
FWIW, I merged these two patches into the audit#next branch because
they are contained to audit and have value regardless of what else
happens during this development cycle. It is just linux-next after
all, not Linus tree so if I need to drop the patches later I can do
that easily enough. I'd rather get more exposure to the patches than
less, and getting into linux-next now helps that.
--
paul moore
http://www.paul-moore.com
On Fri, 2015-02-20 at 08:28 -0800, Davidlohr Bueso wrote:
> 8<--------------------------------------------------------------------
> Subject: [PATCH v2 3/3] tomoyo: reduce mmap_sem hold for mm->exe_file
Tetsuo, could you please ack/nack this?
Davidlohr Bueso wrote:
> On Fri, 2015-02-20 at 07:11 +0900, Tetsuo Handa wrote:
> > Davidlohr Bueso wrote:
> > > On Thu, 2015-02-19 at 20:07 +0900, Tetsuo Handa wrote:
> > > > Why do we need to let the caller call path_put() ?
> > > > There is no need to do like proc_exe_link() does, for
> > > > tomoyo_get_exe() returns pathname as "char *".
> > >
> > > Having the pathname doesn't guarantee anything later, and thus doesn't
> > > seem very robust in the manager call if it can be dropped during the
> > > call... or can this never occur in this context?
> > >
> > tomoyo_get_exe() returns the pathname of executable of current thread.
> > The executable of current thread cannot be changed while current thread
> > is inside the manager call. Although the pathname of executable of
> > current thread could be changed by other threads via namespace manipulation
> > like pivot_root(), holding a reference guarantees nothing. Your patch helps
> > for avoiding memory allocation with mmap_sem held, but does not robustify
> > handling of mm->exe_file for tomoyo.
>
> Fair enough, I won't argue. This is beyond the scope if what I'm trying
> to accomplish here anyway. Are you ok with this instead?
>
I prefer cleanups excluded from this patch, like shown below.
Would you please resend?
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index e0fb750..73ce629 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -908,6 +908,31 @@ static void tomoyo_read_manager(struct tomoyo_io_buffer *head)
> }
>
> /**
> + * tomoyo_get_exe - Get tomoyo_realpath() of current process.
> + *
> + * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
> + *
> + * This function uses kzalloc(), so the caller must call kfree()
> + * if this function didn't return NULL.
> + */
> +static const char *tomoyo_get_exe(void)
> +{
> + struct file *exe_file;
> + const char *cp = NULL;
No need to initialize cp here.
> + struct mm_struct *mm = current->mm;
> +
> + if (!mm)
> + return NULL;
> + exe_file = get_mm_exe_file(mm);
> + if (!exe_file)
> + return NULL;
> +
> + cp = tomoyo_realpath_from_path(&exe_file->f_path);
> + fput(exe_file);
> + return cp;
> +}
> +
> +/**
> * tomoyo_manager - Check whether the current process is a policy manager.
> *
> * Returns true if the current process is permitted to modify policy
> diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
> index b897d48..fc89eba 100644
> --- a/security/tomoyo/common.h
> +++ b/security/tomoyo/common.h
> @@ -947,7 +947,6 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
> char *tomoyo_read_token(struct tomoyo_acl_param *param);
> char *tomoyo_realpath_from_path(struct path *path);
> char *tomoyo_realpath_nofollow(const char *pathname);
> -const char *tomoyo_get_exe(void);
> const char *tomoyo_yesno(const unsigned int value);
> const struct tomoyo_path_info *tomoyo_compare_name_union
> (const struct tomoyo_path_info *name, const struct tomoyo_name_union *ptr);
> diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> index 2952ba5..7eff479 100644
> --- a/security/tomoyo/util.c
> +++ b/security/tomoyo/util.c
> @@ -939,28 +939,6 @@ bool tomoyo_path_matches_pattern(const struct tomoyo_path_info *filename,
> }
>
> /**
> - * tomoyo_get_exe - Get tomoyo_realpath() of current process.
> - *
> - * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
> - *
> - * This function uses kzalloc(), so the caller must call kfree()
> - * if this function didn't return NULL.
> - */
> -const char *tomoyo_get_exe(void)
> -{
> - struct mm_struct *mm = current->mm;
> - const char *cp = NULL;
> -
> - if (!mm)
> - return NULL;
> - down_read(&mm->mmap_sem);
> - if (mm->exe_file)
> - cp = tomoyo_realpath_from_path(&mm->exe_file->f_path);
> - up_read(&mm->mmap_sem);
> - return cp;
> -}
> -
> -/**
> * tomoyo_get_mode - Get MAC mode.
> *
> * @ns: Pointer to "struct tomoyo_policy_namespace".
> --
> 2.1.4
You can post TOMOYO patches to [email protected] .
I'll add your mail address to ML's accept list.
The mm->exe_file is currently serialized with mmap_sem (shared) in order
to both safely (1) read the file and (2) compute the realpath by calling
tomoyo_realpath_from_path, making it an absolute overkill. Good users will,
on the other hand, make use of the more standard get_mm_exe_file(), requiring
only holding the mmap_sem to read the value, and relying on reference
Signed-off-by: Davidlohr Bueso <[email protected]>
---
Changes from v2: remove cleanups and cp initialization.
security/tomoyo/util.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 2952ba5..29f3b65 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -948,16 +948,19 @@ bool tomoyo_path_matches_pattern(const struct tomoyo_path_info *filename,
*/
const char *tomoyo_get_exe(void)
{
- struct mm_struct *mm = current->mm;
- const char *cp = NULL;
+ struct file *exe_file;
+ const char *cp;
+ struct mm_struct *mm = current->mm;
- if (!mm)
- return NULL;
- down_read(&mm->mmap_sem);
- if (mm->exe_file)
- cp = tomoyo_realpath_from_path(&mm->exe_file->f_path);
- up_read(&mm->mmap_sem);
- return cp;
+ if (!mm)
+ return NULL;
+ exe_file = get_mm_exe_file(mm);
+ if (!exe_file)
+ return NULL;
+
+ cp = tomoyo_realpath_from_path(&exe_file->f_path);
+ fput(exe_file);
+ return cp;
}
/**
--
2.1.4
Davidlohr Bueso wrote:
> The mm->exe_file is currently serialized with mmap_sem (shared) in order
> to both safely (1) read the file and (2) compute the realpath by calling
> tomoyo_realpath_from_path, making it an absolute overkill. Good users will,
> on the other hand, make use of the more standard get_mm_exe_file(), requiring
> only holding the mmap_sem to read the value, and relying on reference
>
> Signed-off-by: Davidlohr Bueso <[email protected]>
Acked-by: Tetsuo Handa <[email protected]>
James, will you apply to linux-security.git#next ?
I'm not using publicly accessible git tree for sending pull requests.
> ---
>
> Changes from v2: remove cleanups and cp initialization.
>
> security/tomoyo/util.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> index 2952ba5..29f3b65 100644
> --- a/security/tomoyo/util.c
> +++ b/security/tomoyo/util.c
> @@ -948,16 +948,19 @@ bool tomoyo_path_matches_pattern(const struct tomoyo_path_info *filename,
> */
> const char *tomoyo_get_exe(void)
> {
> - struct mm_struct *mm = current->mm;
> - const char *cp = NULL;
> + struct file *exe_file;
> + const char *cp;
> + struct mm_struct *mm = current->mm;
>
> - if (!mm)
> - return NULL;
> - down_read(&mm->mmap_sem);
> - if (mm->exe_file)
> - cp = tomoyo_realpath_from_path(&mm->exe_file->f_path);
> - up_read(&mm->mmap_sem);
> - return cp;
> + if (!mm)
> + return NULL;
> + exe_file = get_mm_exe_file(mm);
> + if (!exe_file)
> + return NULL;
> +
> + cp = tomoyo_realpath_from_path(&exe_file->f_path);
> + fput(exe_file);
> + return cp;
> }
>
> /**
> --
> 2.1.4
>
On Wed, 2015-02-25 at 20:40 +0900, Tetsuo Handa wrote:
> Davidlohr Bueso wrote:
> > The mm->exe_file is currently serialized with mmap_sem (shared) in order
> > to both safely (1) read the file and (2) compute the realpath by calling
> > tomoyo_realpath_from_path, making it an absolute overkill. Good users will,
> > on the other hand, make use of the more standard get_mm_exe_file(), requiring
> > only holding the mmap_sem to read the value, and relying on reference
> >
> > Signed-off-by: Davidlohr Bueso <[email protected]>
>
> Acked-by: Tetsuo Handa <[email protected]>
>
> James, will you apply to linux-security.git#next ?
> I'm not using publicly accessible git tree for sending pull requests.
I'm actually trying to route these through Andrew. Because there will be
lock conversions, I'm afraid that if such patches are merged in
different order to Linus' tree, it will break bisectibility as you'd
have races.
Thanks,
Davidlohr