lkml.org 
[lkml]   [2020]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@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.

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

--
paul moore
www.paul-moore.com

\
 
 \ /
  Last update: 2020-07-14 22:30    [W:0.080 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site