[RFC PATCH] audit: use current whenever possible
Richard Guy Briggs
rgb at redhat.com
Mon Jul 23 19:37:33 UTC 2018
On 2018-07-20 18:17, Paul Moore wrote:
> 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?
> Signed-off-by: Paul Moore <paul at paul-moore.com>
Otherwise, this cleanup looks like a good simplification.
Reviewed-by: Richard Guy Briggs <rgb at redhat.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(¤t->sighand->siglock, flags);
> + if (current->signal)
> + tty = tty_kref_get(current->signal->tty);
> + spin_unlock_irqrestore(¤t->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;
>
> --
> Linux-audit mailing list
> Linux-audit at redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rgb at 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
More information about the Linux-audit
mailing list