[RFC PATCH] audit: minimize our use of audit_log_format()
Richard Guy Briggs
rgb at redhat.com
Fri Aug 3 00:02:25 UTC 2018
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.
For what my opinion's worth...
Reviewed-by: Richard Guy Briggs <rgb at redhat.com>
> ---
> 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
More information about the Linux-audit
mailing list