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 <[email protected]> Otherwise, this cleanup looks like a good simplification. Reviewed-by: Richard Guy Briggs <[email protected]> > --- > 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 > [email protected] > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <[email protected]> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
