Messages in this thread | | | From | Paul Moore <> | Date | Tue, 21 Jul 2020 17:16:24 -0400 | Subject | Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API |
| |
On Tue, Jul 21, 2020 at 3:31 PM John Johansen <john.johansen@canonical.com> wrote: > 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.
Yeah, I was kind of on the fence about requiring a new version from Richard. I think "?" is arguably the right approach, but I don't think it matters enough to force the issue. If it proves to be problematic we can fix it later.
Regardless, it's in audit/next now.
-- paul moore www.paul-moore.com
| |