Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API

On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2020-07-14 12:21, Paul Moore wrote: > > On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > > > --- > > > Passes audit-testsuite. > > > > > > Changelog: > > > v4 > > > - use double quotes in all replaced audit_log_string() calls > > > > > > v3 > > > - fix two warning: non-void function does not return a value in all control paths > > > Reported-by: kernel test robot <lkp(a)intel.com&gt; > > > > > > 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/dbbcba46335a002f44b058741... > > > > > > 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(-) > > > > Thanks for restoring the quotes, just one question below ... > > > > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c > > > index 4ecedffbdd33..fe36d112aad9 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 ""; > > > > Are we okay with this returning an empty string ("") in this case? > > Should it be a question mark ("?")? > > > > My guess is that userspace parsing should be okay since it still has > > quotes, I'm just not sure if we wanted to use a question mark as we do > > in other cases where the field value is empty/unknown. > > Previously, it would have been an empty value, not even double quotes. > "?" might be an improvement. Did you want to fix that now in this patch, or leave it to later? As I said above, I'm not too bothered by it with the quotes so either way is fine by me.

John, I'm assuming you are okay with this patch? -- paul moore