lkml.org 
[lkml]   [2020]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API
From
Date
On 7/21/20 8:19 AM, Paul Moore wrote:
> On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>> On 2020-07-14 16:29, Paul Moore wrote:
>>> On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> On 2020-07-14 12:21, Paul Moore wrote:
>>>>> On Mon, Jul 13, 2020 at 3:52 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:
>>>>>> 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@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(-)
>>>>>
>>>>> 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.

\
 
 \ /
  Last update: 2020-07-21 21:32    [W:1.191 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site