On Tue, Jul 21, 2020 at 3:31 PM John Johansen
[off-list ref] wrote:

On 7/21/20 8:19 AM, Paul Moore wrote:

quoted

On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs [off-list ref] wrote:

quoted

On 2020-07-14 16:29, Paul Moore wrote:

quoted

On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs [off-list ref] wrote:

quoted

On 2020-07-14 12:21, Paul Moore wrote:

quoted

On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs [off-list ref] wrote:

quoted

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 <redacted>
---
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 [off-list ref]

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 ...

quoted

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.

quoted

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