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

On 7/21/20 8:19 AM, Paul Moore wrote: > 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. sorry, for some reason I thought a new iteration of this was coming. the patch is fine, the empty unknown value should be possible here so changing it to "?" won't affect anything.