[RFC PATCH] audit: minimize our use of audit_log

Discussion:

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

Paul Moore

2018-08-02 22:24:27 UTC

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-moore.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);
}

Richard Guy Briggs

2018-08-03 00:02:25 UTC

Permalink

Post by Paul Moore
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.

This all looks reasonable to me.

For what my opinion's worth...

Post by Paul Moore
---
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)
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) {
/* log the full path */
- audit_log_format(ab, " name=");
+ audit_log_string(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
@@ -2144,12 +2144,12 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
break;
/* 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) {
- audit_log_format(ab, "NORMAL");
+ audit_log_string(ab, "nametype=NORMAL");
break;
- audit_log_format(ab, "PARENT");
+ audit_log_string(ab, "nametype=PARENT");
break;
- audit_log_format(ab, "DELETE");
+ audit_log_string(ab, "nametype=DELETE");
break;
- audit_log_format(ab, "CREATE");
+ audit_log_string(ab, "nametype=CREATE");
break;
- 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;
- 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;
- 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
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <***@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

2018-08-03 01:59:02 UTC

Permalink

Post by Richard Guy Briggs

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.

Post by Richard Guy Briggs
For what my opinion's worth...

I'm always happy to get additional eyes on patches.

Post by Richard Guy Briggs

- RGB
--
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

Ondrej Mosnacek

2018-08-03 06:59:18 UTC

Permalink

FWIW,

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.