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

On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2020-07-14 16:29, Paul Moore wrote: >> 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. > > I'd defer to Steve, otherwise I'd say leave it, since there wasn't > anything there before and this makes that more evident. > >> John, I'm assuming you are okay with this patch? With no comments from John or Steve in the past week, I've gone ahead and merged the patch into audit/next.