[RFC PATCH] audit: minimize our use of audit_log_format()
Paul Moore
paul at paul-moore.com
Fri Aug 3 01:59:02 UTC 2018
On Thu, Aug 2, 2018 at 8:05 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> 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 at paul-moore.com>
>
> This all looks reasonable to me.
Thanks for the review. I still need to actually build a kernel with
it and make sure I didn't do something stupid, but like the current
(ref: task_struct) RFC, I'll probably revisit this after the merge
window.
> For what my opinion's worth...
> Reviewed-by: Richard Guy Briggs <rgb at redhat.com>
I'm always happy to get additional eyes on patches.
> > ---
> > 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 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
--
paul moore
www.paul-moore.com
More information about the Linux-audit
mailing list