Re: [RFC PATCH] audit: minimize our use of audit_log_format()

On 2018-08-02 18:24, Paul Moore wrote: > WARNING: completely untested patch! > > There are several cases where we are using audit_log_format() when we > could be using audit_log_string(), which should be quicker. There are > also some cases where we are making multiple audit_log_format() calls > in a row, for no apparent reason. > > This patch fixes the problems above in the core audit code, the other > kernel subsystems are left for another time. > > Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; This all looks reasonable to me.

For what my opinion's worth... Reviewed-by: Richard Guy Briggs <rgb(a)redhat.com&gt;

> --- > kernel/audit.c | 37 ++++++++++++++++++------------------- > kernel/audit_fsnotify.c | 2 +- > kernel/audit_tree.c | 3 +-- > kernel/audit_watch.c | 2 +- > kernel/auditsc.c | 19 +++++++++---------- > 5 files changed, 30 insertions(+), 33 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 160144f7e5f9..a0f57f4f9944 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1347,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > else { > int size; > > - audit_log_format(ab, " data="); > + audit_log_string(ab, " data="); > size = nlmsg_len(nlh); > if (size > 0 && > ((unsigned char *)data)[size - 1] == '\0') > @@ -1375,7 +1375,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > case AUDIT_TRIM: > audit_trim_trees(); > audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE); > - audit_log_format(ab, " op=trim res=1"); > + audit_log_string(ab, " op=trim res=1"); > audit_log_end(ab); > break; > case AUDIT_MAKE_EQUIV: { > @@ -1406,9 +1406,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > > audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE); > > - audit_log_format(ab, " op=make_equiv old="); > + audit_log_string(ab, " op=make_equiv old="); > audit_log_untrustedstring(ab, old); > - audit_log_format(ab, " new="); > + audit_log_string(ab, " new="); > audit_log_untrustedstring(ab, new); > audit_log_format(ab, " res=%d", !err); > audit_log_end(ab); > @@ -2021,7 +2021,7 @@ void audit_log_d_path(struct audit_buffer *ab, const char *prefix, > char *p, *pathname; > > if (prefix) > - audit_log_format(ab, "%s", prefix); > + audit_log_string(ab, prefix); > > /* We will allow 11 spaces for ' (deleted)' to be appended */ > pathname = kmalloc(PATH_MAX+11, ab->gfp_mask); > @@ -2048,11 +2048,11 @@ void audit_log_session_info(struct audit_buffer *ab) > > void audit_log_key(struct audit_buffer *ab, char *key) > { > - audit_log_format(ab, " key="); > + audit_log_string(ab, " key="); > if (key) > audit_log_untrustedstring(ab, key); > else > - audit_log_format(ab, "(null)"); > + audit_log_string(ab, "(null)"); > } > > void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap) > @@ -2134,7 +2134,7 @@ void audit_log_name(struct audit_context *context, struct audit_names *n, > switch (n->name_len) { > case AUDIT_NAME_FULL: > /* log the full path */ > - audit_log_format(ab, " name="); > + audit_log_string(ab, " name="); > audit_log_untrustedstring(ab, n->name->name); > break; > case 0: > @@ -2144,12 +2144,12 @@ void audit_log_name(struct audit_context *context, struct audit_names *n, > break; > default: > /* log the name's directory component */ > - audit_log_format(ab, " name="); > + audit_log_string(ab, " name="); > audit_log_n_untrustedstring(ab, n->name->name, > n->name_len); > } > } else > - audit_log_format(ab, " name=(null)"); > + audit_log_string(ab, " name=(null)"); > > if (n->ino != AUDIT_INO_UNSET) > audit_log_format(ab, " inode=%lu" > @@ -2178,22 +2178,21 @@ void audit_log_name(struct audit_context *context, struct audit_names *n, > } > > /* log the audit_names record type */ > - audit_log_format(ab, " nametype="); > switch(n->type) { > case AUDIT_TYPE_NORMAL: > - audit_log_format(ab, "NORMAL"); > + audit_log_string(ab, "nametype=NORMAL"); > break; > case AUDIT_TYPE_PARENT: > - audit_log_format(ab, "PARENT"); > + audit_log_string(ab, "nametype=PARENT"); > break; > case AUDIT_TYPE_CHILD_DELETE: > - audit_log_format(ab, "DELETE"); > + audit_log_string(ab, "nametype=DELETE"); > break; > case AUDIT_TYPE_CHILD_CREATE: > - audit_log_format(ab, "CREATE"); > + audit_log_string(ab, "nametype=CREATE"); > break; > default: > - audit_log_format(ab, "UNKNOWN"); > + audit_log_string(ab, "nametype=UNKNOWN"); > break; > } > > @@ -2245,7 +2244,7 @@ void audit_log_d_path_exe(struct audit_buffer *ab, > fput(exe_file); > return; > out_null: > - audit_log_format(ab, " exe=(null)"); > + audit_log_string(ab, " exe=(null)"); > } > > struct tty_struct *audit_get_tty(void) > @@ -2294,7 +2293,7 @@ void audit_log_task_info(struct audit_buffer *ab) > tty ? tty_name(tty) : "(none)", > audit_get_sessionid(current)); > audit_put_tty(tty); > - audit_log_format(ab, " comm="); > + audit_log_string(ab, " comm="); > audit_log_untrustedstring(ab, get_task_comm(comm, current)); > audit_log_d_path_exe(ab, current->mm); > audit_log_task_context(ab); > @@ -2318,7 +2317,7 @@ void audit_log_link_denied(const char *operation) > return; > audit_log_format(ab, "op=%s", operation); > audit_log_task_info(ab); > - audit_log_format(ab, " res=0"); > + audit_log_string(ab, " res=0"); > audit_log_end(ab); > } > > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c > index fba78047fb37..27d29103333c 100644 > --- a/kernel/audit_fsnotify.c > +++ b/kernel/audit_fsnotify.c > @@ -133,7 +133,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c > audit_log_format(ab, "auid=%u ses=%u op=%s", > from_kuid(&init_user_ns, audit_get_loginuid(current)), > audit_get_sessionid(current), op); > - audit_log_format(ab, " path="); > + audit_log_string(ab, " path="); > audit_log_untrustedstring(ab, audit_mark->path); > audit_log_key(ab, rule->filterkey); > audit_log_format(ab, " list=%d res=1", rule->listnr); > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 9f6eaeb6919f..f01bce6d1b23 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -502,8 +502,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule) > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > if (unlikely(!ab)) > return; > - audit_log_format(ab, "op=remove_rule"); > - audit_log_format(ab, " dir="); > + audit_log_string(ab, "op=remove_rule dir="); > audit_log_untrustedstring(ab, rule->tree->pathname); > audit_log_key(ab, rule->filterkey); > audit_log_format(ab, " list=%d res=1", rule->listnr); > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c > index 787c7afdf829..0ce85fe25a53 100644 > --- a/kernel/audit_watch.c > +++ b/kernel/audit_watch.c > @@ -248,7 +248,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc > audit_log_format(ab, "auid=%u ses=%u op=%s", > from_kuid(&init_user_ns, audit_get_loginuid(current)), > audit_get_sessionid(current), op); > - audit_log_format(ab, " path="); > + audit_log_string(ab, " path="); > audit_log_untrustedstring(ab, w->path); > audit_log_key(ab, r->filterkey); > audit_log_format(ab, " list=%d res=1", r->listnr); > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 8b12e525306e..f370930265ea 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -954,14 +954,14 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid, > from_kuid(&init_user_ns, uid), sessionid); > if (sid) { > if (security_secid_to_secctx(sid, &ctx, &len)) { > - audit_log_format(ab, " obj=(none)"); > + audit_log_string(ab, " obj=(none)"); > rc = 1; > } else { > audit_log_format(ab, " obj=%s", ctx); > security_release_secctx(ctx, len); > } > } > - audit_log_format(ab, " ocomm="); > + audit_log_string(ab, " ocomm="); > audit_log_untrustedstring(ab, comm); > audit_log_end(ab); > > @@ -1104,7 +1104,7 @@ static void audit_log_execve_info(struct audit_context *context, > abuf[sizeof(abuf) - 1] = '\0'; > > /* log the arg in the audit record */ > - audit_log_format(*ab, "%s", abuf); > + audit_log_string(*ab, abuf); > len_rem -= len_tmp; > len_tmp = len_buf; > if (encode) { > @@ -1240,7 +1240,7 @@ static void show_special(struct audit_context *context, int *call_panic) > audit_log_execve_info(context, &ab); > break; > case AUDIT_KERN_MODULE: > - audit_log_format(ab, "name="); > + audit_log_string(ab, "name="); > audit_log_untrustedstring(ab, context->module.name); > kfree(context->module.name); > break; > @@ -1276,7 +1276,7 @@ static void audit_log_proctitle(void) > if (!ab) > return; /* audit_panic or being filtered */ > > - audit_log_format(ab, "proctitle="); > + audit_log_string(ab, "proctitle="); > > /* Not cached */ > if (!context->proctitle.value) { > @@ -1405,7 +1405,7 @@ static void audit_log_exit(int ret_valid, long ret_code) > if (context->sockaddr_len) { > ab = audit_log_start(context, GFP_KERNEL, AUDIT_SOCKADDR); > if (ab) { > - audit_log_format(ab, "saddr="); > + audit_log_string(ab, "saddr="); > audit_log_n_hex(ab, (void *)context->sockaddr, > context->sockaddr_len); > audit_log_end(ab); > @@ -2498,10 +2498,9 @@ void audit_seccomp_actions_logged(const char *names, const char *old_names, > if (unlikely(!ab)) > return; > > - audit_log_format(ab, "op=seccomp-logging"); > - audit_log_format(ab, " actions=%s", names); > - audit_log_format(ab, " old-actions=%s", old_names); > - audit_log_format(ab, " res=%d", res); > + audit_log_string(ab, "op=seccomp-logging"); > + audit_log_format(ab, " actions=%s old-actions=%s res=%d", > + names, old_names, res); > audit_log_end(ab); > } > > > -- > Linux-audit mailing list > Linux-audit(a)redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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