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

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

[Richard Guy Briggs]

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(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 523f77494847..b3d859831a31 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -694,9 +694,4 @@ static inline bool audit_loginuid_set(struct task_struct *tsk)
 	return uid_valid(audit_get_loginuid(tsk));
 }
 
-static inline void audit_log_string(struct audit_buffer *ab, const char *buf)
-{
-	audit_log_n_string(ab, buf, strlen(buf));
-}
-
 #endif
diff --git a/kernel/audit.c b/kernel/audit.c
index 8c201f414226..a2f3e34aa724 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2080,13 +2080,13 @@ void audit_log_d_path(struct audit_buffer *ab, const char *prefix,
 	/* We will allow 11 spaces for ' (deleted)' to be appended */
 	pathname = kmalloc(PATH_MAX+11, ab->gfp_mask);
 	if (!pathname) {
-		audit_log_string(ab, "<no_memory>");
+		audit_log_format(ab, "\"<no_memory>\"");
 		return;
 	}
 	p = d_path(path, pathname, PATH_MAX+11);
 	if (IS_ERR(p)) { /* Should never happen since we send PATH_MAX */
 		/* FIXME: can we save some information here? */
-		audit_log_string(ab, "<too_long>");
+		audit_log_format(ab, "\"<too_long>\"");
 	} else
 		audit_log_untrustedstring(ab, p);
 	kfree(pathname);
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 597732503815..f7e97c7e80f3 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -57,18 +57,16 @@ static void audit_pre(struct audit_buffer *ab, void *ca)
 	struct common_audit_data *sa = ca;
 
 	if (aa_g_audit_header) {
-		audit_log_format(ab, "apparmor=");
-		audit_log_string(ab, aa_audit_type[aad(sa)->type]);
+		audit_log_format(ab, "apparmor=\"%s\"",
+				 aa_audit_type[aad(sa)->type]);
 	}
 
 	if (aad(sa)->op) {
-		audit_log_format(ab, " operation=");
-		audit_log_string(ab, aad(sa)->op);
+		audit_log_format(ab, " operation=\"%s\"", aad(sa)->op);
 	}
 
 	if (aad(sa)->info) {
-		audit_log_format(ab, " info=");
-		audit_log_string(ab, aad(sa)->info);
+		audit_log_format(ab, " info=\"%s\"", aad(sa)->info);
 		if (aad(sa)->error)
 			audit_log_format(ab, " error=%d", aad(sa)->error);
 	}
diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index 9a2d14b7c9f8..92acf9a49405 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -35,20 +35,6 @@ static u32 map_mask_to_chr_mask(u32 mask)
 }
 
 /**
- * audit_file_mask - convert mask to permission string
- * @buffer: buffer to write string to (NOT NULL)
- * @mask: permission mask to convert
- */
-static void audit_file_mask(struct audit_buffer *ab, u32 mask)
-{
-	char str[10];
-
-	aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs,
-			    map_mask_to_chr_mask(mask));
-	audit_log_string(ab, str);
-}
-
-/**
  * file_audit_cb - call back for file specific audit fields
  * @ab: audit_buffer  (NOT NULL)
  * @va: audit struct to audit values of  (NOT NULL)
@@ -57,14 +43,17 @@ static void file_audit_cb(struct audit_buffer *ab, void *va)
 {
 	struct common_audit_data *sa = va;
 	kuid_t fsuid = current_fsuid();
+	char str[10];
 
 	if (aad(sa)->request & AA_AUDIT_FILE_MASK) {
-		audit_log_format(ab, " requested_mask=");
-		audit_file_mask(ab, aad(sa)->request);
+		aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs,
+				    map_mask_to_chr_mask(aad(sa)->request));
+		audit_log_format(ab, " requested_mask=\"%s\"", str);
 	}
 	if (aad(sa)->denied & AA_AUDIT_FILE_MASK) {
-		audit_log_format(ab, " denied_mask=");
-		audit_file_mask(ab, aad(sa)->denied);
+		aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs,
+				    map_mask_to_chr_mask(aad(sa)->denied));
+		audit_log_format(ab, " denied_mask=\"%s\"", str);
 	}
 	if (aad(sa)->request & AA_AUDIT_FILE_MASK) {
 		audit_log_format(ab, " fsuid=%d",
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 "";
 }
 
 /* call back to audit ptrace fields */
@@ -47,12 +45,12 @@ static void audit_ptrace_cb(struct audit_buffer *ab, void *va)
 	struct common_audit_data *sa = va;
 
 	if (aad(sa)->request & AA_PTRACE_PERM_MASK) {
-		audit_log_format(ab, " requested_mask=");
-		audit_ptrace_mask(ab, aad(sa)->request);
+		audit_log_format(ab, " requested_mask=\"%s\"",
+				 audit_ptrace_mask(aad(sa)->request));
 
 		if (aad(sa)->denied & AA_PTRACE_PERM_MASK) {
-			audit_log_format(ab, " denied_mask=");
-			audit_ptrace_mask(ab, aad(sa)->denied);
+			audit_log_format(ab, " denied_mask=\"%s\"",
+					 audit_ptrace_mask(aad(sa)->denied));
 		}
 	}
 	audit_log_format(ab, " peer=");
@@ -142,16 +140,18 @@ static inline int map_signal_num(int sig)
 }
 
 /**
- * audit_file_mask - convert mask to permission string
- * @buffer: buffer to write string to (NOT NULL)
+ * audit_signal_mask - convert mask to permission string
  * @mask: permission mask to convert
+ *
+ * Returns: pointer to static string
  */
-static void audit_signal_mask(struct audit_buffer *ab, u32 mask)
+static const char *audit_signal_mask(u32 mask)
 {
 	if (mask & MAY_READ)
-		audit_log_string(ab, "receive");
+		return "receive";
 	if (mask & MAY_WRITE)
-		audit_log_string(ab, "send");
+		return "send";
+	return "";
 }
 
 /**
@@ -164,11 +164,11 @@ static void audit_signal_cb(struct audit_buffer *ab, void *va)
 	struct common_audit_data *sa = va;
 
 	if (aad(sa)->request & AA_SIGNAL_PERM_MASK) {
-		audit_log_format(ab, " requested_mask=");
-		audit_signal_mask(ab, aad(sa)->request);
+		audit_log_format(ab, " requested_mask=\"%s\"",
+				 audit_signal_mask(aad(sa)->request));
 		if (aad(sa)->denied & AA_SIGNAL_PERM_MASK) {
-			audit_log_format(ab, " denied_mask=");
-			audit_signal_mask(ab, aad(sa)->denied);
+			audit_log_format(ab, " denied_mask=\"%s\"",
+					 audit_signal_mask(aad(sa)->denied));
 		}
 	}
 	if (aad(sa)->signal == SIGUNKNOWN)
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index d8afc39f663a..fa0e85568450 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -72,16 +72,18 @@ void audit_net_cb(struct audit_buffer *ab, void *va)
 {
 	struct common_audit_data *sa = va;
 
-	audit_log_format(ab, " family=");
 	if (address_family_names[sa->u.net->family])
-		audit_log_string(ab, address_family_names[sa->u.net->family]);
+		audit_log_format(ab, " family=\"%s\"",
+				 address_family_names[sa->u.net->family]);
 	else
-		audit_log_format(ab, "\"unknown(%d)\"", sa->u.net->family);
-	audit_log_format(ab, " sock_type=");
+		audit_log_format(ab, " family=\"unknown(%d)\"",
+				 sa->u.net->family);
 	if (sock_type_names[aad(sa)->net.type])
-		audit_log_string(ab, sock_type_names[aad(sa)->net.type]);
+		audit_log_format(ab, " sock_type=\"%s\"",
+				 sock_type_names[aad(sa)->net.type]);
 	else
-		audit_log_format(ab, "\"unknown(%d)\"", aad(sa)->net.type);
+		audit_log_format(ab, " sock_type=\"unknown(%d)\"",
+				 aad(sa)->net.type);
 	audit_log_format(ab, " protocol=%d", aad(sa)->net.protocol);
 
 	if (aad(sa)->request & NET_PERMS_MASK) {
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 7c555621c2bd..53d0d183db8f 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -432,8 +432,8 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 				 a->u.ibendport->port);
 		break;
 	case LSM_AUDIT_DATA_LOCKDOWN:
-		audit_log_format(ab, " lockdown_reason=");
-		audit_log_string(ab, lockdown_reasons[a->u.reason]);
+		audit_log_format(ab, " lockdown_reason=\"%s\"",
+				 lockdown_reasons[a->u.reason]);
 		break;
 	} /* switch (a->type) */
 }
-- 
1.8.3.1

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

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

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.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Paul Moore]

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

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

[Richard Guy Briggs]

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?
> 
> -- 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com

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

[John Johansen]

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.

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

[Paul Moore]

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