Discussion:
[RFC PATCH] audit: use current whenever possible
Paul Moore
2018-07-20 22:17:10 UTC
Permalink
There are many places, notably audit_log_task_info() and
audit_log_exit(), that take task_struct pointers but in reality they
are always working on the current task. This patch eliminates the
task_struct arguments and uses current directly which allows a number
of cleanups as well.

Signed-off-by: Paul Moore <***@paul-moore.com>
---
drivers/tty/tty_audit.c | 13 ++--
include/linux/audit.h | 6 +-
kernel/audit.c | 34 +++++------
kernel/audit.h | 2 -
kernel/auditsc.c | 118 +++++++++++++++++---------------------
security/integrity/ima/ima_api.c | 2 -
6 files changed, 81 insertions(+), 94 deletions(-)

diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 50f567b6a66e..28f87fd6a28e 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -61,20 +61,19 @@ static void tty_audit_log(const char *description, dev_t dev,
unsigned char *data, size_t size)
{
struct audit_buffer *ab;
- struct task_struct *tsk = current;
- pid_t pid = task_pid_nr(tsk);
- uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
- uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
- unsigned int sessionid = audit_get_sessionid(tsk);
+ pid_t pid = task_pid_nr(current);
+ uid_t uid = from_kuid(&init_user_ns, task_uid(current));
+ uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
+ unsigned int sessionid = audit_get_sessionid(current);

ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
if (ab) {
- char name[sizeof(tsk->comm)];
+ char name[sizeof(current->comm)];

audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d"
" minor=%d comm=", description, pid, uid,
loginuid, sessionid, MAJOR(dev), MINOR(dev));
- get_task_comm(name, tsk);
+ get_task_comm(name, current);
audit_log_untrustedstring(ab, name);
audit_log_format(ab, " data=");
audit_log_n_hex(ab, data, size);
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9334fbef7bae..108dd9706a30 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -153,8 +153,7 @@ extern void audit_log_link_denied(const char *operation);
extern void audit_log_lost(const char *message);

extern int audit_log_task_context(struct audit_buffer *ab);
-extern void audit_log_task_info(struct audit_buffer *ab,
- struct task_struct *tsk);
+extern void audit_log_task_info(struct audit_buffer *ab);

extern int audit_update_lsm_rules(void);

@@ -202,8 +201,7 @@ static inline int audit_log_task_context(struct audit_buffer *ab)
{
return 0;
}
-static inline void audit_log_task_info(struct audit_buffer *ab,
- struct task_struct *tsk)
+static inline void audit_log_task_info(struct audit_buffer *ab)
{ }
#define audit_enabled AUDIT_OFF
#endif /* CONFIG_AUDIT */
diff --git a/kernel/audit.c b/kernel/audit.c
index 2a8058764aa6..160144f7e5f9 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1096,10 +1096,11 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature

if (audit_enabled == AUDIT_OFF)
return;
+
ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FEATURE_CHANGE);
if (!ab)
return;
- audit_log_task_info(ab, current);
+ audit_log_task_info(ab);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
!!old_lock, !!new_lock, res);
@@ -2247,15 +2248,15 @@ void audit_log_d_path_exe(struct audit_buffer *ab,
audit_log_format(ab, " exe=(null)");
}

-struct tty_struct *audit_get_tty(struct task_struct *tsk)
+struct tty_struct *audit_get_tty(void)
{
struct tty_struct *tty = NULL;
unsigned long flags;

- spin_lock_irqsave(&tsk->sighand->siglock, flags);
- if (tsk->signal)
- tty = tty_kref_get(tsk->signal->tty);
- spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+ spin_lock_irqsave(&current->sighand->siglock, flags);
+ if (current->signal)
+ tty = tty_kref_get(current->signal->tty);
+ spin_unlock_irqrestore(&current->sighand->siglock, flags);
return tty;
}

@@ -2264,25 +2265,24 @@ void audit_put_tty(struct tty_struct *tty)
tty_kref_put(tty);
}

-void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
+void audit_log_task_info(struct audit_buffer *ab)
{
const struct cred *cred;
- char comm[sizeof(tsk->comm)];
+ char comm[sizeof(current->comm)];
struct tty_struct *tty;

if (!ab)
return;

- /* tsk == current */
cred = current_cred();
- tty = audit_get_tty(tsk);
+ tty = audit_get_tty();
audit_log_format(ab,
" ppid=%d pid=%d auid=%u uid=%u gid=%u"
" euid=%u suid=%u fsuid=%u"
" egid=%u sgid=%u fsgid=%u tty=%s ses=%u",
- task_ppid_nr(tsk),
- task_tgid_nr(tsk),
- from_kuid(&init_user_ns, audit_get_loginuid(tsk)),
+ task_ppid_nr(current),
+ task_tgid_nr(current),
+ from_kuid(&init_user_ns, audit_get_loginuid(current)),
from_kuid(&init_user_ns, cred->uid),
from_kgid(&init_user_ns, cred->gid),
from_kuid(&init_user_ns, cred->euid),
@@ -2292,11 +2292,11 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
from_kgid(&init_user_ns, cred->sgid),
from_kgid(&init_user_ns, cred->fsgid),
tty ? tty_name(tty) : "(none)",
- audit_get_sessionid(tsk));
+ audit_get_sessionid(current));
audit_put_tty(tty);
audit_log_format(ab, " comm=");
- audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
- audit_log_d_path_exe(ab, tsk->mm);
+ audit_log_untrustedstring(ab, get_task_comm(comm, current));
+ audit_log_d_path_exe(ab, current->mm);
audit_log_task_context(ab);
}
EXPORT_SYMBOL(audit_log_task_info);
@@ -2317,7 +2317,7 @@ void audit_log_link_denied(const char *operation)
if (!ab)
return;
audit_log_format(ab, "op=%s", operation);
- audit_log_task_info(ab, current);
+ audit_log_task_info(ab);
audit_log_format(ab, " res=0");
audit_log_end(ab);
}
diff --git a/kernel/audit.h b/kernel/audit.h
index 214e14948370..9c76b7a6e956 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -262,7 +262,7 @@ 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);

-extern struct tty_struct *audit_get_tty(struct task_struct *tsk);
+extern struct tty_struct *audit_get_tty(void);
extern void audit_put_tty(struct tty_struct *tty);

/* audit watch functions */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fb207466e99b..8b12e525306e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -836,44 +836,6 @@ void audit_filter_inodes(struct task_struct *tsk, struct audit_context *ctx)
rcu_read_unlock();
}

-/* Transfer the audit context pointer to the caller, clearing it in the tsk's struct */
-static inline struct audit_context *audit_take_context(struct task_struct *tsk,
- int return_valid,
- long return_code)
-{
- struct audit_context *context = tsk->audit_context;
-
- if (!context)
- return NULL;
- context->return_valid = return_valid;
-
- /*
- * we need to fix up the return code in the audit logs if the actual
- * return codes are later going to be fixed up by the arch specific
- * signal handlers
- *
- * This is actually a test for:
- * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) ||
- * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK)
- *
- * but is faster than a bunch of ||
- */
- if (unlikely(return_code <= -ERESTARTSYS) &&
- (return_code >= -ERESTART_RESTARTBLOCK) &&
- (return_code != -ENOIOCTLCMD))
- context->return_code = -EINTR;
- else
- context->return_code = return_code;
-
- if (context->in_syscall && !context->dummy) {
- audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_EXIT]);
- audit_filter_inodes(tsk, context);
- }
-
- audit_set_context(tsk, NULL);
- return context;
-}
-
static inline void audit_proctitle_free(struct audit_context *context)
{
kfree(context->proctitle.value);
@@ -1298,15 +1260,18 @@ static inline int audit_proctitle_rtrim(char *proctitle, int len)
return len;
}

-static void audit_log_proctitle(struct task_struct *tsk,
- struct audit_context *context)
+static void audit_log_proctitle(void)
{
int res;
char *buf;
char *msg = "(null)";
int len = strlen(msg);
+ struct audit_context *context = audit_context();
struct audit_buffer *ab;

+ if (!context)
+ return;
+
ab = audit_log_start(context, GFP_KERNEL, AUDIT_PROCTITLE);
if (!ab)
return; /* audit_panic or being filtered */
@@ -1319,7 +1284,7 @@ static void audit_log_proctitle(struct task_struct *tsk,
if (!buf)
goto out;
/* Historically called this from procfs naming */
- res = get_cmdline(tsk, buf, MAX_PROCTITLE_AUDIT_LEN);
+ res = get_cmdline(current, buf, MAX_PROCTITLE_AUDIT_LEN);
if (res == 0) {
kfree(buf);
goto out;
@@ -1339,15 +1304,39 @@ static void audit_log_proctitle(struct task_struct *tsk,
audit_log_end(ab);
}

-static void audit_log_exit(struct audit_context *context, struct task_struct *tsk)
+static void audit_log_exit(int ret_valid, long ret_code)
{
int i, call_panic = 0;
+ struct audit_context *context = audit_context();
struct audit_buffer *ab;
struct audit_aux_data *aux;
struct audit_names *n;

- /* tsk == current */
- context->personality = tsk->personality;
+ context->personality = current->personality;
+ context->return_valid = ret_valid;
+
+ /*
+ * we need to fix up the return code in the audit logs if the actual
+ * return codes are later going to be fixed up by the arch specific
+ * signal handlers
+ *
+ * This is actually a test for:
+ * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) ||
+ * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK)
+ *
+ * but is faster than a bunch of ||
+ */
+ if (unlikely(ret_code <= -ERESTARTSYS) &&
+ (ret_code >= -ERESTART_RESTARTBLOCK) &&
+ (ret_code != -ENOIOCTLCMD))
+ ret_code = -EINTR;
+ context->return_code = ret_code;
+
+ if (context->in_syscall && !context->dummy) {
+ audit_filter_syscall(current, context,
+ &audit_filter_list[AUDIT_FILTER_EXIT]);
+ audit_filter_inodes(current, context);
+ }

ab = audit_log_start(context, GFP_KERNEL, AUDIT_SYSCALL);
if (!ab)
@@ -1356,10 +1345,10 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
context->arch, context->major);
if (context->personality != PER_LINUX)
audit_log_format(ab, " per=%lx", context->personality);
- if (context->return_valid)
+ if (ret_valid)
audit_log_format(ab, " success=%s exit=%ld",
- (context->return_valid==AUDITSC_SUCCESS)?"yes":"no",
- context->return_code);
+ (ret_valid == AUDITSC_SUCCESS) ? "yes" : "no",
+ ret_code);

audit_log_format(ab,
" a0=%lx a1=%lx a2=%lx a3=%lx items=%d",
@@ -1369,7 +1358,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
context->argv[3],
context->name_count);

- audit_log_task_info(ab, tsk);
+ audit_log_task_info(ab);
audit_log_key(ab, context->filterkey);
audit_log_end(ab);

@@ -1458,7 +1447,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
audit_log_name(context, n, NULL, i++, &call_panic);
}

- audit_log_proctitle(tsk, context);
+ audit_log_proctitle();

/* Send end of event record to help user space know we are finished */
ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
@@ -1478,20 +1467,23 @@ void __audit_free(struct task_struct *tsk)
{
struct audit_context *context;

- context = audit_take_context(tsk, 0, 0);
+ context = tsk->audit_context;
if (!context)
return;

- /* Check for system calls that do not go through the exit
- * function (e.g., exit_group), then free context block.
- * We use GFP_ATOMIC here because we might be doing this
- * in the context of the idle thread */
- /* that can happen only if we are called from do_exit() */
- if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
- audit_log_exit(context, tsk);
+ /* We are called either by do_exit() or the fork() error handling code;
+ * in the former case tsk == current and in the latter tsk is a
+ * random task_struct that doesn't doesn't have any meaningful data we
+ * need to log via audit_log_exit(). */
+ if (tsk == current &&
+ context->in_syscall &&
+ context->current_state == AUDIT_RECORD_CONTEXT)
+ audit_log_exit(0, 0);
+
if (!list_empty(&context->killed_trees))
audit_kill_trees(&context->killed_trees);

+ audit_set_context(tsk, NULL);
audit_free_context(context);
}

@@ -1566,12 +1558,12 @@ void __audit_syscall_exit(int success, long return_code)
else
success = AUDITSC_FAILURE;

- context = audit_take_context(current, success, return_code);
+ context = audit_context();
if (!context)
return;

if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
- audit_log_exit(context, current);
+ audit_log_exit(success, return_code);

context->in_syscall = 0;
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
@@ -1593,7 +1585,6 @@ void __audit_syscall_exit(int success, long return_code)
kfree(context->filterkey);
context->filterkey = NULL;
}
- audit_set_context(current, context);
}

static inline void handle_one(const struct inode *inode)
@@ -2031,7 +2022,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
uid = from_kuid(&init_user_ns, task_uid(current));
oldloginuid = from_kuid(&init_user_ns, koldloginuid);
loginuid = from_kuid(&init_user_ns, kloginuid),
- tty = audit_get_tty(current);
+ tty = audit_get_tty();

audit_log_format(ab, "pid=%d uid=%u", task_tgid_nr(current), uid);
audit_log_task_context(ab);
@@ -2052,7 +2043,6 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
*/
int audit_set_loginuid(kuid_t loginuid)
{
- struct task_struct *task = current;
unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
kuid_t oldloginuid;
int rc;
@@ -2071,8 +2061,8 @@ int audit_set_loginuid(kuid_t loginuid)
sessionid = (unsigned int)atomic_inc_return(&session_id);
}

- task->sessionid = sessionid;
- task->loginuid = loginuid;
+ current->sessionid = sessionid;
+ current->loginuid = loginuid;
out:
audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
return rc;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index a02c5acfd403..30999169c0a8 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -335,7 +335,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_untrustedstring(ab, filename);
audit_log_format(ab, " hash=\"%s:%s\"", algo_name, hash);

- audit_log_task_info(ab, current);
+ audit_log_task_info(ab);
audit_log_end(ab);

iint->flags |= IMA_AUDITED;
Richard Guy Briggs
2018-07-23 19:37:33 UTC
Permalink
Post by Paul Moore
There are many places, notably audit_log_task_info() and
audit_log_exit(), that take task_struct pointers but in reality they
are always working on the current task. This patch eliminates the
task_struct arguments and uses current directly which allows a number
of cleanups as well.
I came across and removed a several in the audit task struct cleanup,
but it looks like you've rebased over those and caught a few more.

I'm fine with delaying setting task's context to NULL for
__audit_free().

Why was the context originally taken for __audit_syscall_exit() and
given back once the syscall event records have been issued? Is there a
possible race with something else?
Otherwise, this cleanup looks like a good simplification.
Post by Paul Moore
---
drivers/tty/tty_audit.c | 13 ++--
include/linux/audit.h | 6 +-
kernel/audit.c | 34 +++++------
kernel/audit.h | 2 -
kernel/auditsc.c | 118 +++++++++++++++++---------------------
security/integrity/ima/ima_api.c | 2 -
6 files changed, 81 insertions(+), 94 deletions(-)
diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 50f567b6a66e..28f87fd6a28e 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -61,20 +61,19 @@ static void tty_audit_log(const char *description, dev_t dev,
unsigned char *data, size_t size)
{
struct audit_buffer *ab;
- struct task_struct *tsk = current;
- pid_t pid = task_pid_nr(tsk);
- uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
- uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
- unsigned int sessionid = audit_get_sessionid(tsk);
+ pid_t pid = task_pid_nr(current);
+ uid_t uid = from_kuid(&init_user_ns, task_uid(current));
+ uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
+ unsigned int sessionid = audit_get_sessionid(current);
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
if (ab) {
- char name[sizeof(tsk->comm)];
+ char name[sizeof(current->comm)];
audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d"
" minor=%d comm=", description, pid, uid,
loginuid, sessionid, MAJOR(dev), MINOR(dev));
- get_task_comm(name, tsk);
+ get_task_comm(name, current);
audit_log_untrustedstring(ab, name);
audit_log_format(ab, " data=");
audit_log_n_hex(ab, data, size);
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9334fbef7bae..108dd9706a30 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -153,8 +153,7 @@ extern void audit_log_link_denied(const char *operation);
extern void audit_log_lost(const char *message);
extern int audit_log_task_context(struct audit_buffer *ab);
-extern void audit_log_task_info(struct audit_buffer *ab,
- struct task_struct *tsk);
+extern void audit_log_task_info(struct audit_buffer *ab);
extern int audit_update_lsm_rules(void);
@@ -202,8 +201,7 @@ static inline int audit_log_task_context(struct audit_buffer *ab)
{
return 0;
}
-static inline void audit_log_task_info(struct audit_buffer *ab,
- struct task_struct *tsk)
+static inline void audit_log_task_info(struct audit_buffer *ab)
{ }
#define audit_enabled AUDIT_OFF
#endif /* CONFIG_AUDIT */
diff --git a/kernel/audit.c b/kernel/audit.c
index 2a8058764aa6..160144f7e5f9 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1096,10 +1096,11 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
if (audit_enabled == AUDIT_OFF)
return;
+
ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FEATURE_CHANGE);
if (!ab)
return;
- audit_log_task_info(ab, current);
+ audit_log_task_info(ab);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
!!old_lock, !!new_lock, res);
@@ -2247,15 +2248,15 @@ void audit_log_d_path_exe(struct audit_buffer *ab,
audit_log_format(ab, " exe=(null)");
}
-struct tty_struct *audit_get_tty(struct task_struct *tsk)
+struct tty_struct *audit_get_tty(void)
{
struct tty_struct *tty = NULL;
unsigned long flags;
- spin_lock_irqsave(&tsk->sighand->siglock, flags);
- if (tsk->signal)
- tty = tty_kref_get(tsk->signal->tty);
- spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+ spin_lock_irqsave(&current->sighand->siglock, flags);
+ if (current->signal)
+ tty = tty_kref_get(current->signal->tty);
+ spin_unlock_irqrestore(&current->sighand->siglock, flags);
return tty;
}
@@ -2264,25 +2265,24 @@ void audit_put_tty(struct tty_struct *tty)
tty_kref_put(tty);
}
-void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
+void audit_log_task_info(struct audit_buffer *ab)
{
const struct cred *cred;
- char comm[sizeof(tsk->comm)];
+ char comm[sizeof(current->comm)];
struct tty_struct *tty;
if (!ab)
return;
- /* tsk == current */
cred = current_cred();
- tty = audit_get_tty(tsk);
+ tty = audit_get_tty();
audit_log_format(ab,
" ppid=%d pid=%d auid=%u uid=%u gid=%u"
" euid=%u suid=%u fsuid=%u"
" egid=%u sgid=%u fsgid=%u tty=%s ses=%u",
- task_ppid_nr(tsk),
- task_tgid_nr(tsk),
- from_kuid(&init_user_ns, audit_get_loginuid(tsk)),
+ task_ppid_nr(current),
+ task_tgid_nr(current),
+ from_kuid(&init_user_ns, audit_get_loginuid(current)),
from_kuid(&init_user_ns, cred->uid),
from_kgid(&init_user_ns, cred->gid),
from_kuid(&init_user_ns, cred->euid),
@@ -2292,11 +2292,11 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
from_kgid(&init_user_ns, cred->sgid),
from_kgid(&init_user_ns, cred->fsgid),
tty ? tty_name(tty) : "(none)",
- audit_get_sessionid(tsk));
+ audit_get_sessionid(current));
audit_put_tty(tty);
audit_log_format(ab, " comm=");
- audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
- audit_log_d_path_exe(ab, tsk->mm);
+ audit_log_untrustedstring(ab, get_task_comm(comm, current));
+ audit_log_d_path_exe(ab, current->mm);
audit_log_task_context(ab);
}
EXPORT_SYMBOL(audit_log_task_info);
@@ -2317,7 +2317,7 @@ void audit_log_link_denied(const char *operation)
if (!ab)
return;
audit_log_format(ab, "op=%s", operation);
- audit_log_task_info(ab, current);
+ audit_log_task_info(ab);
audit_log_format(ab, " res=0");
audit_log_end(ab);
}
diff --git a/kernel/audit.h b/kernel/audit.h
index 214e14948370..9c76b7a6e956 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -262,7 +262,7 @@ 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);
-extern struct tty_struct *audit_get_tty(struct task_struct *tsk);
+extern struct tty_struct *audit_get_tty(void);
extern void audit_put_tty(struct tty_struct *tty);
/* audit watch functions */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fb207466e99b..8b12e525306e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -836,44 +836,6 @@ void audit_filter_inodes(struct task_struct *tsk, struct audit_context *ctx)
rcu_read_unlock();
}
-/* Transfer the audit context pointer to the caller, clearing it in the tsk's struct */
-static inline struct audit_context *audit_take_context(struct task_struct *tsk,
- int return_valid,
- long return_code)
-{
- struct audit_context *context = tsk->audit_context;
-
- if (!context)
- return NULL;
- context->return_valid = return_valid;
-
- /*
- * we need to fix up the return code in the audit logs if the actual
- * return codes are later going to be fixed up by the arch specific
- * signal handlers
- *
- * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) ||
- * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK)
- *
- * but is faster than a bunch of ||
- */
- if (unlikely(return_code <= -ERESTARTSYS) &&
- (return_code >= -ERESTART_RESTARTBLOCK) &&
- (return_code != -ENOIOCTLCMD))
- context->return_code = -EINTR;
- else
- context->return_code = return_code;
-
- if (context->in_syscall && !context->dummy) {
- audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_EXIT]);
- audit_filter_inodes(tsk, context);
- }
-
- audit_set_context(tsk, NULL);
- return context;
-}
-
static inline void audit_proctitle_free(struct audit_context *context)
{
kfree(context->proctitle.value);
@@ -1298,15 +1260,18 @@ static inline int audit_proctitle_rtrim(char *proctitle, int len)
return len;
}
-static void audit_log_proctitle(struct task_struct *tsk,
- struct audit_context *context)
+static void audit_log_proctitle(void)
{
int res;
char *buf;
char *msg = "(null)";
int len = strlen(msg);
+ struct audit_context *context = audit_context();
struct audit_buffer *ab;
+ if (!context)
+ return;
+
ab = audit_log_start(context, GFP_KERNEL, AUDIT_PROCTITLE);
if (!ab)
return; /* audit_panic or being filtered */
@@ -1319,7 +1284,7 @@ static void audit_log_proctitle(struct task_struct *tsk,
if (!buf)
goto out;
/* Historically called this from procfs naming */
- res = get_cmdline(tsk, buf, MAX_PROCTITLE_AUDIT_LEN);
+ res = get_cmdline(current, buf, MAX_PROCTITLE_AUDIT_LEN);
if (res == 0) {
kfree(buf);
goto out;
@@ -1339,15 +1304,39 @@ static void audit_log_proctitle(struct task_struct *tsk,
audit_log_end(ab);
}
-static void audit_log_exit(struct audit_context *context, struct task_struct *tsk)
+static void audit_log_exit(int ret_valid, long ret_code)
{
int i, call_panic = 0;
+ struct audit_context *context = audit_context();
struct audit_buffer *ab;
struct audit_aux_data *aux;
struct audit_names *n;
- /* tsk == current */
- context->personality = tsk->personality;
+ context->personality = current->personality;
+ context->return_valid = ret_valid;
+
+ /*
+ * we need to fix up the return code in the audit logs if the actual
+ * return codes are later going to be fixed up by the arch specific
+ * signal handlers
+ *
+ * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) ||
+ * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK)
+ *
+ * but is faster than a bunch of ||
+ */
+ if (unlikely(ret_code <= -ERESTARTSYS) &&
+ (ret_code >= -ERESTART_RESTARTBLOCK) &&
+ (ret_code != -ENOIOCTLCMD))
+ ret_code = -EINTR;
+ context->return_code = ret_code;
+
+ if (context->in_syscall && !context->dummy) {
+ audit_filter_syscall(current, context,
+ &audit_filter_list[AUDIT_FILTER_EXIT]);
+ audit_filter_inodes(current, context);
+ }
ab = audit_log_start(context, GFP_KERNEL, AUDIT_SYSCALL);
if (!ab)
@@ -1356,10 +1345,10 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
context->arch, context->major);
if (context->personality != PER_LINUX)
audit_log_format(ab, " per=%lx", context->personality);
- if (context->return_valid)
+ if (ret_valid)
audit_log_format(ab, " success=%s exit=%ld",
- (context->return_valid==AUDITSC_SUCCESS)?"yes":"no",
- context->return_code);
+ (ret_valid == AUDITSC_SUCCESS) ? "yes" : "no",
+ ret_code);
audit_log_format(ab,
" a0=%lx a1=%lx a2=%lx a3=%lx items=%d",
@@ -1369,7 +1358,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
context->argv[3],
context->name_count);
- audit_log_task_info(ab, tsk);
+ audit_log_task_info(ab);
audit_log_key(ab, context->filterkey);
audit_log_end(ab);
@@ -1458,7 +1447,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
audit_log_name(context, n, NULL, i++, &call_panic);
}
- audit_log_proctitle(tsk, context);
+ audit_log_proctitle();
/* Send end of event record to help user space know we are finished */
ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
@@ -1478,20 +1467,23 @@ void __audit_free(struct task_struct *tsk)
{
struct audit_context *context;
- context = audit_take_context(tsk, 0, 0);
+ context = tsk->audit_context;
if (!context)
return;
- /* Check for system calls that do not go through the exit
- * function (e.g., exit_group), then free context block.
- * We use GFP_ATOMIC here because we might be doing this
- * in the context of the idle thread */
- /* that can happen only if we are called from do_exit() */
- if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
- audit_log_exit(context, tsk);
+ /* We are called either by do_exit() or the fork() error handling code;
+ * in the former case tsk == current and in the latter tsk is a
+ * random task_struct that doesn't doesn't have any meaningful data we
+ * need to log via audit_log_exit(). */
+ if (tsk == current &&
+ context->in_syscall &&
+ context->current_state == AUDIT_RECORD_CONTEXT)
+ audit_log_exit(0, 0);
+
if (!list_empty(&context->killed_trees))
audit_kill_trees(&context->killed_trees);
+ audit_set_context(tsk, NULL);
audit_free_context(context);
}
@@ -1566,12 +1558,12 @@ void __audit_syscall_exit(int success, long return_code)
else
success = AUDITSC_FAILURE;
- context = audit_take_context(current, success, return_code);
+ context = audit_context();
if (!context)
return;
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
- audit_log_exit(context, current);
+ audit_log_exit(success, return_code);
context->in_syscall = 0;
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
@@ -1593,7 +1585,6 @@ void __audit_syscall_exit(int success, long return_code)
kfree(context->filterkey);
context->filterkey = NULL;
}
- audit_set_context(current, context);
}
static inline void handle_one(const struct inode *inode)
@@ -2031,7 +2022,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
uid = from_kuid(&init_user_ns, task_uid(current));
oldloginuid = from_kuid(&init_user_ns, koldloginuid);
loginuid = from_kuid(&init_user_ns, kloginuid),
- tty = audit_get_tty(current);
+ tty = audit_get_tty();
audit_log_format(ab, "pid=%d uid=%u", task_tgid_nr(current), uid);
audit_log_task_context(ab);
@@ -2052,7 +2043,6 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
*/
int audit_set_loginuid(kuid_t loginuid)
{
- struct task_struct *task = current;
unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
kuid_t oldloginuid;
int rc;
@@ -2071,8 +2061,8 @@ int audit_set_loginuid(kuid_t loginuid)
sessionid = (unsigned int)atomic_inc_return(&session_id);
}
- task->sessionid = sessionid;
- task->loginuid = loginuid;
+ current->sessionid = sessionid;
+ current->loginuid = loginuid;
audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
return rc;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index a02c5acfd403..30999169c0a8 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -335,7 +335,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_untrustedstring(ab, filename);
audit_log_format(ab, " hash=\"%s:%s\"", algo_name, hash);
- audit_log_task_info(ab, current);
+ audit_log_task_info(ab);
audit_log_end(ab);
iint->flags |= IMA_AUDITED;
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Paul Moore
2018-07-23 21:24:23 UTC
Permalink
Post by Richard Guy Briggs
Post by Paul Moore
There are many places, notably audit_log_task_info() and
audit_log_exit(), that take task_struct pointers but in reality they
are always working on the current task. This patch eliminates the
task_struct arguments and uses current directly which allows a number
of cleanups as well.
I came across and removed a several in the audit task struct cleanup,
but it looks like you've rebased over those and caught a few more.
I just based this patch against audit/next to make life easier. Since
the earliest it would possibly go into the audit tree would be after
the next merge window it will likely get rebased/merged again. If
there is another patch that does some of this work and gets merged
first, awesome, if not, that's fine too.
Post by Richard Guy Briggs
I'm fine with delaying setting task's context to NULL for
__audit_free().
Yeah, it really shouldn't matter when it happens in __audit_free() as
we should be the only ones who are touching that task_struct at that
point in time.
Post by Richard Guy Briggs
Why was the context originally taken for __audit_syscall_exit() and
given back once the syscall event records have been issued? Is there a
possible race with something else?
That was a bit bizarre, wasn't it? There shouldn't be a race
condition as the audit_context is private to the individual task and
at the point in time where __audit_syscall_exit() is being called we
shouldn't have to worry about other things hitting the task_struct.
If anything, this patch should actually make things better by not
setting the current->context to NULL at the start of
__audit_syscall_exit() only to reset it back to the original value at
the end (the audit_take_context() function, and it's relationship with
audit_log_exit() was ... odd ... and that is me being kind).

I'm chalking this up to "audit being audit" :/
Post by Richard Guy Briggs
Otherwise, this cleanup looks like a good simplification.
Diffstats that remove more lines than they add always make me happy.

Thanks for taking a look. It boots and passes our tests but I still
haven't convinced myself all those changes are correct. I'll send a
note if/when it gets merged, but like I said that won't happen until
after the merge window closes as we are at -rc6 right now.
--
paul moore
www.paul-moore.com
Loading...