| Message ID | 20597345545df186f0a287c974c9dc88b5c940a0.1593808662.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ghak84,v3] audit: purge audit_log_string from the intra-kernel audit API | expand |
On Fri, Jul 3, 2020 at 5:50 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > audit_log_string() was inteded to be an internal audit function and > since there are only two internal uses, remove them. Purge all external > uses of it by restructuring code to use an existing audit_log_format() > or using audit_log_format(). > > Please see the upstream issue > https://github.com/linux-audit/audit-kernel/issues/84 > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > Passes audit-testsuite. > > Changelog: > v3 > - fix two warning: non-void function does not return a value in all control paths > Reported-by: kernel test robot <lkp@intel.com> > > v2 > - restructure to piggyback on existing audit_log_format() calls, checking quoting needs for each. > > v1 Vlad Dronov > - https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf > > include/linux/audit.h | 5 ----- > kernel/audit.c | 4 ++-- > security/apparmor/audit.c | 10 ++++------ > security/apparmor/file.c | 25 +++++++------------------ > security/apparmor/ipc.c | 46 +++++++++++++++++++++++----------------------- > security/apparmor/net.c | 14 ++++++++------ > security/lsm_audit.c | 4 ++-- > 7 files changed, 46 insertions(+), 62 deletions(-) ... > diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c > index 597732503815..335b5b8d300b 100644 > --- a/security/apparmor/audit.c > +++ b/security/apparmor/audit.c > @@ -57,18 +57,16 @@ static void audit_pre(struct audit_buffer *ab, void *ca) > struct common_audit_data *sa = ca; > > if (aa_g_audit_header) { > - audit_log_format(ab, "apparmor="); > - audit_log_string(ab, aa_audit_type[aad(sa)->type]); > + audit_log_format(ab, "apparmor=%s", > + aa_audit_type[aad(sa)->type]); > } > > if (aad(sa)->op) { > - audit_log_format(ab, " operation="); > - audit_log_string(ab, aad(sa)->op); > + audit_log_format(ab, " operation=%s", aad(sa)->op); > } In the case below you've added the quotes around the string, but they appear to be missing in the two cases above. > if (aad(sa)->info) { > - audit_log_format(ab, " info="); > - audit_log_string(ab, aad(sa)->info); > + audit_log_format(ab, " info=\"%s\"", aad(sa)->info); > if (aad(sa)->error) > audit_log_format(ab, " error=%d", aad(sa)->error); > } > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > index 9a2d14b7c9f8..70f27124d051 100644 > --- a/security/apparmor/file.c > +++ b/security/apparmor/file.c > @@ -35,20 +35,6 @@ static u32 map_mask_to_chr_mask(u32 mask) > } > > /** > - * audit_file_mask - convert mask to permission string > - * @buffer: buffer to write string to (NOT NULL) > - * @mask: permission mask to convert > - */ > -static void audit_file_mask(struct audit_buffer *ab, u32 mask) > -{ > - char str[10]; > - > - aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > - map_mask_to_chr_mask(mask)); > - audit_log_string(ab, str); > -} > - > -/** > * file_audit_cb - call back for file specific audit fields > * @ab: audit_buffer (NOT NULL) > * @va: audit struct to audit values of (NOT NULL) > @@ -57,14 +43,17 @@ static void file_audit_cb(struct audit_buffer *ab, void *va) > { > struct common_audit_data *sa = va; > kuid_t fsuid = current_fsuid(); > + char str[10]; > > if (aad(sa)->request & AA_AUDIT_FILE_MASK) { > - audit_log_format(ab, " requested_mask="); > - audit_file_mask(ab, aad(sa)->request); > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > + map_mask_to_chr_mask(aad(sa)->request)); > + audit_log_format(ab, " requested_mask=%s", str); > } > if (aad(sa)->denied & AA_AUDIT_FILE_MASK) { > - audit_log_format(ab, " denied_mask="); > - audit_file_mask(ab, aad(sa)->denied); > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > + map_mask_to_chr_mask(aad(sa)->denied)); > + audit_log_format(ab, " denied_mask=%s", str); > } More missing quotes. > if (aad(sa)->request & AA_AUDIT_FILE_MASK) { > audit_log_format(ab, " fsuid=%d", > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c > index 4ecedffbdd33..fe431731883f 100644 > --- a/security/apparmor/ipc.c > +++ b/security/apparmor/ipc.c > @@ -20,25 +20,23 @@ > > /** > * audit_ptrace_mask - convert mask to permission string > - * @buffer: buffer to write string to (NOT NULL) > * @mask: permission mask to convert > + * > + * Returns: pointer to static string > */ > -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask) > +static const char *audit_ptrace_mask(u32 mask) > { > switch (mask) { > case MAY_READ: > - audit_log_string(ab, "read"); > - break; > + return "read"; > case MAY_WRITE: > - audit_log_string(ab, "trace"); > - break; > + return "trace"; > case AA_MAY_BE_READ: > - audit_log_string(ab, "readby"); > - break; > + return "readby"; > case AA_MAY_BE_TRACED: > - audit_log_string(ab, "tracedby"); > - break; > + return "tracedby"; > } > + return ""; > } > > /* call back to audit ptrace fields */ > @@ -47,12 +45,12 @@ static void audit_ptrace_cb(struct audit_buffer *ab, void *va) > struct common_audit_data *sa = va; > > if (aad(sa)->request & AA_PTRACE_PERM_MASK) { > - audit_log_format(ab, " requested_mask="); > - audit_ptrace_mask(ab, aad(sa)->request); > + audit_log_format(ab, " requested_mask=%s", > + audit_ptrace_mask(aad(sa)->request)); > > if (aad(sa)->denied & AA_PTRACE_PERM_MASK) { > - audit_log_format(ab, " denied_mask="); > - audit_ptrace_mask(ab, aad(sa)->denied); > + audit_log_format(ab, " denied_mask=%s", > + audit_ptrace_mask(aad(sa)->denied)); > } Quotes. There are none. ... and it looks like there are more missing too, but I kinda stopped seriously reading the patch here, please take a closer look at the patch, make the necessary changes, and resubmit.
On 2020-07-08 18:41, Paul Moore wrote: > On Fri, Jul 3, 2020 at 5:50 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > audit_log_string() was inteded to be an internal audit function and > > since there are only two internal uses, remove them. Purge all external > > uses of it by restructuring code to use an existing audit_log_format() > > or using audit_log_format(). > > > > Please see the upstream issue > > https://github.com/linux-audit/audit-kernel/issues/84 > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > Passes audit-testsuite. > > > > Changelog: > > v3 > > - fix two warning: non-void function does not return a value in all control paths > > Reported-by: kernel test robot <lkp@intel.com> > > > > v2 > > - restructure to piggyback on existing audit_log_format() calls, checking quoting needs for each. > > > > v1 Vlad Dronov > > - https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf > > > > include/linux/audit.h | 5 ----- > > kernel/audit.c | 4 ++-- > > security/apparmor/audit.c | 10 ++++------ > > security/apparmor/file.c | 25 +++++++------------------ > > security/apparmor/ipc.c | 46 +++++++++++++++++++++++----------------------- > > security/apparmor/net.c | 14 ++++++++------ > > security/lsm_audit.c | 4 ++-- > > 7 files changed, 46 insertions(+), 62 deletions(-) > > ... > > > diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c > > index 597732503815..335b5b8d300b 100644 > > --- a/security/apparmor/audit.c > > +++ b/security/apparmor/audit.c > > @@ -57,18 +57,16 @@ static void audit_pre(struct audit_buffer *ab, void *ca) > > struct common_audit_data *sa = ca; > > > > if (aa_g_audit_header) { > > - audit_log_format(ab, "apparmor="); > > - audit_log_string(ab, aa_audit_type[aad(sa)->type]); > > + audit_log_format(ab, "apparmor=%s", > > + aa_audit_type[aad(sa)->type]); > > } > > > > if (aad(sa)->op) { > > - audit_log_format(ab, " operation="); > > - audit_log_string(ab, aad(sa)->op); > > + audit_log_format(ab, " operation=%s", aad(sa)->op); > > } > > In the case below you've added the quotes around the string, but they > appear to be missing in the two cases above. They aren't needed above since they are text with no spaces or special characters. We don't usually include double quotes where they aren't needed. The one below contains spaces so needs double quotes. > > if (aad(sa)->info) { > > - audit_log_format(ab, " info="); > > - audit_log_string(ab, aad(sa)->info); > > + audit_log_format(ab, " info=\"%s\"", aad(sa)->info); > > if (aad(sa)->error) > > audit_log_format(ab, " error=%d", aad(sa)->error); > > } > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > > index 9a2d14b7c9f8..70f27124d051 100644 > > --- a/security/apparmor/file.c > > +++ b/security/apparmor/file.c > > @@ -35,20 +35,6 @@ static u32 map_mask_to_chr_mask(u32 mask) > > } > > > > /** > > - * audit_file_mask - convert mask to permission string > > - * @buffer: buffer to write string to (NOT NULL) > > - * @mask: permission mask to convert > > - */ > > -static void audit_file_mask(struct audit_buffer *ab, u32 mask) > > -{ > > - char str[10]; > > - > > - aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > > - map_mask_to_chr_mask(mask)); > > - audit_log_string(ab, str); > > -} > > - > > -/** > > * file_audit_cb - call back for file specific audit fields > > * @ab: audit_buffer (NOT NULL) > > * @va: audit struct to audit values of (NOT NULL) > > @@ -57,14 +43,17 @@ static void file_audit_cb(struct audit_buffer *ab, void *va) > > { > > struct common_audit_data *sa = va; > > kuid_t fsuid = current_fsuid(); > > + char str[10]; > > > > if (aad(sa)->request & AA_AUDIT_FILE_MASK) { > > - audit_log_format(ab, " requested_mask="); > > - audit_file_mask(ab, aad(sa)->request); > > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > > + map_mask_to_chr_mask(aad(sa)->request)); > > + audit_log_format(ab, " requested_mask=%s", str); > > } > > if (aad(sa)->denied & AA_AUDIT_FILE_MASK) { > > - audit_log_format(ab, " denied_mask="); > > - audit_file_mask(ab, aad(sa)->denied); > > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > > + map_mask_to_chr_mask(aad(sa)->denied)); > > + audit_log_format(ab, " denied_mask=%s", str); > > } > > More missing quotes. This is a simple text field without punctuation or special characters. > > if (aad(sa)->request & AA_AUDIT_FILE_MASK) { > > audit_log_format(ab, " fsuid=%d", > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c > > index 4ecedffbdd33..fe431731883f 100644 > > --- a/security/apparmor/ipc.c > > +++ b/security/apparmor/ipc.c > > @@ -20,25 +20,23 @@ > > > > /** > > * audit_ptrace_mask - convert mask to permission string > > - * @buffer: buffer to write string to (NOT NULL) > > * @mask: permission mask to convert > > + * > > + * Returns: pointer to static string > > */ > > -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask) > > +static const char *audit_ptrace_mask(u32 mask) > > { > > switch (mask) { > > case MAY_READ: > > - audit_log_string(ab, "read"); > > - break; > > + return "read"; > > case MAY_WRITE: > > - audit_log_string(ab, "trace"); > > - break; > > + return "trace"; > > case AA_MAY_BE_READ: > > - audit_log_string(ab, "readby"); > > - break; > > + return "readby"; > > case AA_MAY_BE_TRACED: > > - audit_log_string(ab, "tracedby"); > > - break; > > + return "tracedby"; > > } > > + return ""; > > } > > > > /* call back to audit ptrace fields */ > > @@ -47,12 +45,12 @@ static void audit_ptrace_cb(struct audit_buffer *ab, void *va) > > struct common_audit_data *sa = va; > > > > if (aad(sa)->request & AA_PTRACE_PERM_MASK) { > > - audit_log_format(ab, " requested_mask="); > > - audit_ptrace_mask(ab, aad(sa)->request); > > + audit_log_format(ab, " requested_mask=%s", > > + audit_ptrace_mask(aad(sa)->request)); > > > > if (aad(sa)->denied & AA_PTRACE_PERM_MASK) { > > - audit_log_format(ab, " denied_mask="); > > - audit_ptrace_mask(ab, aad(sa)->denied); > > + audit_log_format(ab, " denied_mask=%s", > > + audit_ptrace_mask(aad(sa)->denied)); > > } > > Quotes. There are none. > > ... and it looks like there are more missing too, but I kinda stopped > seriously reading the patch here, please take a closer look at the > patch, make the necessary changes, and resubmit. This was all intentional as per the previous patch review discussion on github. > paul moore - RGB -- Richard Guy Briggs <rgb@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
On Wed, Jul 8, 2020 at 7:15 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-07-08 18:41, Paul Moore wrote: > > On Fri, Jul 3, 2020 at 5:50 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > audit_log_string() was inteded to be an internal audit function and > > > since there are only two internal uses, remove them. Purge all external > > > uses of it by restructuring code to use an existing audit_log_format() > > > or using audit_log_format(). > > > > > > Please see the upstream issue > > > https://github.com/linux-audit/audit-kernel/issues/84 > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > Passes audit-testsuite. > > > > > > Changelog: > > > v3 > > > - fix two warning: non-void function does not return a value in all control paths > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > > v2 > > > - restructure to piggyback on existing audit_log_format() calls, checking quoting needs for each. > > > > > > v1 Vlad Dronov > > > - https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf > > > > > > include/linux/audit.h | 5 ----- > > > kernel/audit.c | 4 ++-- > > > security/apparmor/audit.c | 10 ++++------ > > > security/apparmor/file.c | 25 +++++++------------------ > > > security/apparmor/ipc.c | 46 +++++++++++++++++++++++----------------------- > > > security/apparmor/net.c | 14 ++++++++------ > > > security/lsm_audit.c | 4 ++-- > > > 7 files changed, 46 insertions(+), 62 deletions(-) > > > > ... > > > > > diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c > > > index 597732503815..335b5b8d300b 100644 > > > --- a/security/apparmor/audit.c > > > +++ b/security/apparmor/audit.c > > > @@ -57,18 +57,16 @@ static void audit_pre(struct audit_buffer *ab, void *ca) > > > struct common_audit_data *sa = ca; > > > > > > if (aa_g_audit_header) { > > > - audit_log_format(ab, "apparmor="); > > > - audit_log_string(ab, aa_audit_type[aad(sa)->type]); > > > + audit_log_format(ab, "apparmor=%s", > > > + aa_audit_type[aad(sa)->type]); > > > } > > > > > > if (aad(sa)->op) { > > > - audit_log_format(ab, " operation="); > > > - audit_log_string(ab, aad(sa)->op); > > > + audit_log_format(ab, " operation=%s", aad(sa)->op); > > > } > > > > In the case below you've added the quotes around the string, but they > > appear to be missing in the two cases above. > > They aren't needed above since they are text with no spaces or special > characters. We don't usually include double quotes where they aren't > needed. The one below contains spaces so needs double quotes. > > > > if (aad(sa)->info) { > > > - audit_log_format(ab, " info="); > > > - audit_log_string(ab, aad(sa)->info); > > > + audit_log_format(ab, " info=\"%s\"", aad(sa)->info); > > > if (aad(sa)->error) > > > audit_log_format(ab, " error=%d", aad(sa)->error); > > > } > > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > > > index 9a2d14b7c9f8..70f27124d051 100644 > > > --- a/security/apparmor/file.c > > > +++ b/security/apparmor/file.c > > > @@ -35,20 +35,6 @@ static u32 map_mask_to_chr_mask(u32 mask) > > > } > > > > > > /** > > > - * audit_file_mask - convert mask to permission string > > > - * @buffer: buffer to write string to (NOT NULL) > > > - * @mask: permission mask to convert > > > - */ > > > -static void audit_file_mask(struct audit_buffer *ab, u32 mask) > > > -{ > > > - char str[10]; > > > - > > > - aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > > > - map_mask_to_chr_mask(mask)); > > > - audit_log_string(ab, str); > > > -} > > > - > > > -/** > > > * file_audit_cb - call back for file specific audit fields > > > * @ab: audit_buffer (NOT NULL) > > > * @va: audit struct to audit values of (NOT NULL) > > > @@ -57,14 +43,17 @@ static void file_audit_cb(struct audit_buffer *ab, void *va) > > > { > > > struct common_audit_data *sa = va; > > > kuid_t fsuid = current_fsuid(); > > > + char str[10]; > > > > > > if (aad(sa)->request & AA_AUDIT_FILE_MASK) { > > > - audit_log_format(ab, " requested_mask="); > > > - audit_file_mask(ab, aad(sa)->request); > > > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > > > + map_mask_to_chr_mask(aad(sa)->request)); > > > + audit_log_format(ab, " requested_mask=%s", str); > > > } > > > if (aad(sa)->denied & AA_AUDIT_FILE_MASK) { > > > - audit_log_format(ab, " denied_mask="); > > > - audit_file_mask(ab, aad(sa)->denied); > > > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > > > + map_mask_to_chr_mask(aad(sa)->denied)); > > > + audit_log_format(ab, " denied_mask=%s", str); > > > } > > > > More missing quotes. > > This is a simple text field without punctuation or special characters. > > > > if (aad(sa)->request & AA_AUDIT_FILE_MASK) { > > > audit_log_format(ab, " fsuid=%d", > > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c > > > index 4ecedffbdd33..fe431731883f 100644 > > > --- a/security/apparmor/ipc.c > > > +++ b/security/apparmor/ipc.c > > > @@ -20,25 +20,23 @@ > > > > > > /** > > > * audit_ptrace_mask - convert mask to permission string > > > - * @buffer: buffer to write string to (NOT NULL) > > > * @mask: permission mask to convert > > > + * > > > + * Returns: pointer to static string > > > */ > > > -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask) > > > +static const char *audit_ptrace_mask(u32 mask) > > > { > > > switch (mask) { > > > case MAY_READ: > > > - audit_log_string(ab, "read"); > > > - break; > > > + return "read"; > > > case MAY_WRITE: > > > - audit_log_string(ab, "trace"); > > > - break; > > > + return "trace"; > > > case AA_MAY_BE_READ: > > > - audit_log_string(ab, "readby"); > > > - break; > > > + return "readby"; > > > case AA_MAY_BE_TRACED: > > > - audit_log_string(ab, "tracedby"); > > > - break; > > > + return "tracedby"; > > > } > > > + return ""; > > > } > > > > > > /* call back to audit ptrace fields */ > > > @@ -47,12 +45,12 @@ static void audit_ptrace_cb(struct audit_buffer *ab, void *va) > > > struct common_audit_data *sa = va; > > > > > > if (aad(sa)->request & AA_PTRACE_PERM_MASK) { > > > - audit_log_format(ab, " requested_mask="); > > > - audit_ptrace_mask(ab, aad(sa)->request); > > > + audit_log_format(ab, " requested_mask=%s", > > > + audit_ptrace_mask(aad(sa)->request)); > > > > > > if (aad(sa)->denied & AA_PTRACE_PERM_MASK) { > > > - audit_log_format(ab, " denied_mask="); > > > - audit_ptrace_mask(ab, aad(sa)->denied); > > > + audit_log_format(ab, " denied_mask=%s", > > > + audit_ptrace_mask(aad(sa)->denied)); > > > } > > > > Quotes. There are none. > > > > ... and it looks like there are more missing too, but I kinda stopped > > seriously reading the patch here, please take a closer look at the > > patch, make the necessary changes, and resubmit. > > This was all intentional as per the previous patch review discussion on github. The GitHub discussion doesn't explicitly address the issue of quoting, and it's on GitHub not in the commit description. Links, while helpful, do not count; the commit descriptions must stand on their own. I generally frown upon patches which change the record formatting, this patch is no exception. Please revise the patch so there is no change and in the records emitted from the kernel and resubmit.
diff --git a/include/linux/audit.h b/include/linux/audit.h index 604ede630580..5ad7cd65d76f 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -695,9 +695,4 @@ static inline bool audit_loginuid_set(struct task_struct *tsk) return uid_valid(audit_get_loginuid(tsk)); } -static inline void audit_log_string(struct audit_buffer *ab, const char *buf) -{ - audit_log_n_string(ab, buf, strlen(buf)); -} - #endif diff --git a/kernel/audit.c b/kernel/audit.c index 8c201f414226..a2f3e34aa724 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -2080,13 +2080,13 @@ void audit_log_d_path(struct audit_buffer *ab, const char *prefix, /* We will allow 11 spaces for ' (deleted)' to be appended */ pathname = kmalloc(PATH_MAX+11, ab->gfp_mask); if (!pathname) { - audit_log_string(ab, "<no_memory>"); + audit_log_format(ab, "\"<no_memory>\""); return; } p = d_path(path, pathname, PATH_MAX+11); if (IS_ERR(p)) { /* Should never happen since we send PATH_MAX */ /* FIXME: can we save some information here? */ - audit_log_string(ab, "<too_long>"); + audit_log_format(ab, "\"<too_long>\""); } else audit_log_untrustedstring(ab, p); kfree(pathname); diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c index 597732503815..335b5b8d300b 100644 --- a/security/apparmor/audit.c +++ b/security/apparmor/audit.c @@ -57,18 +57,16 @@ static void audit_pre(struct audit_buffer *ab, void *ca) struct common_audit_data *sa = ca; if (aa_g_audit_header) { - audit_log_format(ab, "apparmor="); - audit_log_string(ab, aa_audit_type[aad(sa)->type]); + audit_log_format(ab, "apparmor=%s", + aa_audit_type[aad(sa)->type]); } if (aad(sa)->op) { - audit_log_format(ab, " operation="); - audit_log_string(ab, aad(sa)->op); + audit_log_format(ab, " operation=%s", aad(sa)->op); } if (aad(sa)->info) { - audit_log_format(ab, " info="); - audit_log_string(ab, aad(sa)->info); + audit_log_format(ab, " info=\"%s\"", aad(sa)->info); if (aad(sa)->error) audit_log_format(ab, " error=%d", aad(sa)->error); } diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 9a2d14b7c9f8..70f27124d051 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -35,20 +35,6 @@ static u32 map_mask_to_chr_mask(u32 mask) } /** - * audit_file_mask - convert mask to permission string - * @buffer: buffer to write string to (NOT NULL) - * @mask: permission mask to convert - */ -static void audit_file_mask(struct audit_buffer *ab, u32 mask) -{ - char str[10]; - - aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, - map_mask_to_chr_mask(mask)); - audit_log_string(ab, str); -} - -/** * file_audit_cb - call back for file specific audit fields * @ab: audit_buffer (NOT NULL) * @va: audit struct to audit values of (NOT NULL) @@ -57,14 +43,17 @@ static void file_audit_cb(struct audit_buffer *ab, void *va) { struct common_audit_data *sa = va; kuid_t fsuid = current_fsuid(); + char str[10]; if (aad(sa)->request & AA_AUDIT_FILE_MASK) { - audit_log_format(ab, " requested_mask="); - audit_file_mask(ab, aad(sa)->request); + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, + map_mask_to_chr_mask(aad(sa)->request)); + audit_log_format(ab, " requested_mask=%s", str); } if (aad(sa)->denied & AA_AUDIT_FILE_MASK) { - audit_log_format(ab, " denied_mask="); - audit_file_mask(ab, aad(sa)->denied); + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, + map_mask_to_chr_mask(aad(sa)->denied)); + audit_log_format(ab, " denied_mask=%s", str); } if (aad(sa)->request & AA_AUDIT_FILE_MASK) { audit_log_format(ab, " fsuid=%d", diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c index 4ecedffbdd33..fe431731883f 100644 --- a/security/apparmor/ipc.c +++ b/security/apparmor/ipc.c @@ -20,25 +20,23 @@ /** * audit_ptrace_mask - convert mask to permission string - * @buffer: buffer to write string to (NOT NULL) * @mask: permission mask to convert + * + * Returns: pointer to static string */ -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask) +static const char *audit_ptrace_mask(u32 mask) { switch (mask) { case MAY_READ: - audit_log_string(ab, "read"); - break; + return "read"; case MAY_WRITE: - audit_log_string(ab, "trace"); - break; + return "trace"; case AA_MAY_BE_READ: - audit_log_string(ab, "readby"); - break; + return "readby"; case AA_MAY_BE_TRACED: - audit_log_string(ab, "tracedby"); - break; + return "tracedby"; } + return ""; } /* call back to audit ptrace fields */ @@ -47,12 +45,12 @@ static void audit_ptrace_cb(struct audit_buffer *ab, void *va) struct common_audit_data *sa = va; if (aad(sa)->request & AA_PTRACE_PERM_MASK) { - audit_log_format(ab, " requested_mask="); - audit_ptrace_mask(ab, aad(sa)->request); + audit_log_format(ab, " requested_mask=%s", + audit_ptrace_mask(aad(sa)->request)); if (aad(sa)->denied & AA_PTRACE_PERM_MASK) { - audit_log_format(ab, " denied_mask="); - audit_ptrace_mask(ab, aad(sa)->denied); + audit_log_format(ab, " denied_mask=%s", + audit_ptrace_mask(aad(sa)->denied)); } } audit_log_format(ab, " peer="); @@ -142,16 +140,18 @@ static inline int map_signal_num(int sig) } /** - * audit_file_mask - convert mask to permission string - * @buffer: buffer to write string to (NOT NULL) + * audit_signal_mask - convert mask to permission string * @mask: permission mask to convert + * + * Returns: pointer to static string */ -static void audit_signal_mask(struct audit_buffer *ab, u32 mask) +static const char *audit_signal_mask(u32 mask) { if (mask & MAY_READ) - audit_log_string(ab, "receive"); + return "receive"; if (mask & MAY_WRITE) - audit_log_string(ab, "send"); + return "send"; + return ""; } /** @@ -164,11 +164,11 @@ static void audit_signal_cb(struct audit_buffer *ab, void *va) struct common_audit_data *sa = va; if (aad(sa)->request & AA_SIGNAL_PERM_MASK) { - audit_log_format(ab, " requested_mask="); - audit_signal_mask(ab, aad(sa)->request); + audit_log_format(ab, " requested_mask=%s", + audit_signal_mask(aad(sa)->request)); if (aad(sa)->denied & AA_SIGNAL_PERM_MASK) { - audit_log_format(ab, " denied_mask="); - audit_signal_mask(ab, aad(sa)->denied); + audit_log_format(ab, " denied_mask=%s", + audit_signal_mask(aad(sa)->denied)); } } if (aad(sa)->signal == SIGUNKNOWN) diff --git a/security/apparmor/net.c b/security/apparmor/net.c index d8afc39f663a..fa0e85568450 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -72,16 +72,18 @@ void audit_net_cb(struct audit_buffer *ab, void *va) { struct common_audit_data *sa = va; - audit_log_format(ab, " family="); if (address_family_names[sa->u.net->family]) - audit_log_string(ab, address_family_names[sa->u.net->family]); + audit_log_format(ab, " family=\"%s\"", + address_family_names[sa->u.net->family]); else - audit_log_format(ab, "\"unknown(%d)\"", sa->u.net->family); - audit_log_format(ab, " sock_type="); + audit_log_format(ab, " family=\"unknown(%d)\"", + sa->u.net->family); if (sock_type_names[aad(sa)->net.type]) - audit_log_string(ab, sock_type_names[aad(sa)->net.type]); + audit_log_format(ab, " sock_type=\"%s\"", + sock_type_names[aad(sa)->net.type]); else - audit_log_format(ab, "\"unknown(%d)\"", aad(sa)->net.type); + audit_log_format(ab, " sock_type=\"unknown(%d)\"", + aad(sa)->net.type); audit_log_format(ab, " protocol=%d", aad(sa)->net.protocol); if (aad(sa)->request & NET_PERMS_MASK) { diff --git a/security/lsm_audit.c b/security/lsm_audit.c index 2d2bf49016f4..221370794d14 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -427,8 +427,8 @@ static void dump_common_audit_data(struct audit_buffer *ab, a->u.ibendport->port); break; case LSM_AUDIT_DATA_LOCKDOWN: - audit_log_format(ab, " lockdown_reason="); - audit_log_string(ab, lockdown_reasons[a->u.reason]); + audit_log_format(ab, " lockdown_reason=\"%s\"", + lockdown_reasons[a->u.reason]); break; } /* switch (a->type) */ }
audit_log_string() was inteded to be an internal audit function and since there are only two internal uses, remove them. Purge all external uses of it by restructuring code to use an existing audit_log_format() or using audit_log_format(). Please see the upstream issue https://github.com/linux-audit/audit-kernel/issues/84 Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- Passes audit-testsuite. Changelog: v3 - fix two warning: non-void function does not return a value in all control paths Reported-by: kernel test robot <lkp@intel.com> v2 - restructure to piggyback on existing audit_log_format() calls, checking quoting needs for each. v1 Vlad Dronov - https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf include/linux/audit.h | 5 ----- kernel/audit.c | 4 ++-- security/apparmor/audit.c | 10 ++++------ security/apparmor/file.c | 25 +++++++------------------ security/apparmor/ipc.c | 46 +++++++++++++++++++++++----------------------- security/apparmor/net.c | 14 ++++++++------ security/lsm_audit.c | 4 ++-- 7 files changed, 46 insertions(+), 62 deletions(-)