audit: config_change normalizations and event record gathering

The failure to add an audit rule due to audit locked gives no clue what CONFIG_CHANGE operation failed.
Similarly the set operation is the only other operation that doesn't give the "op=" field to indicate the action.
All other CONFIG_CHANGE records include an op= field to give a clue as to what sort of configuration change is being executed.

Since these are the only CONFIG_CHANGE records that that do not have an op= field, add them to bring them in line with the rest.

Old records:
type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0 type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes

New records:
type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0

type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes

See: https://github.com/linux-audit/audit-kernel/issues/59

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/audit.c +4 -3

index 779671883349..0e8026423fbd 100644
@@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
        ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
        if (unlikely(!ab))
                return rc;
-       audit_log_format(ab, "%s=%u old=%u ", function_name, new, old);
+       audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
        audit_log_session_info(ab);
        rc = audit_log_task_context(ab);
        if (rc)

@@ -1363,7 +1363,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                        return -EINVAL;
                if (audit_enabled == AUDIT_LOCKED) {
                        audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
-                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
+                       audit_log_format(ab, " op=%s audit_enabled=%d res=0",
+                                        msg_type == AUDIT_ADD_RULE ? "add_rule" : "remove_rule",
+                                        audit_enabled);
                        audit_log_end(ab);
                        return -EPERM;
                }
--

1.8.3.1

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types, and since the AUDIT_CONFIG_CHANGE message type has been converted to a syscall accompanied record type, special-case the AUDIT_USER_* range of messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c          | 27 +++++++++++++++++++--------
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     |  2 +-
 kernel/audit_watch.c    |  2 +-
 kernel/auditfilter.c    |  2 +-
 5 files changed, 23 insertions(+), 12 deletions(-)

kernel/audit.c +0 -0

index 0e8026423fbd..a321fea94cc6 100644

Quoted 26 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types, and since the AUDIT_CONFIG_CHANGE message type has been converted to a syscall accompanied record type, special-case the AUDIT_USER_* range of messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c          | 27 +++++++++++++++++++--------
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     |  2 +-
 kernel/audit_watch.c    |  2 +-
 kernel/auditfilter.c    |  2 +-
 5 files changed, 23 insertions(+), 12 deletions(-)

kernel/audit.c +0 -0

index 0e8026423fbd..a321fea94cc6 100644

kernel/audit.c static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) · line 1079

Quoted 7 lines

kernel/audit.c +4 -0

@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
        audit_log_task_context(*ab);
 }
 
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+       audit_log_common_recv_msg(NULL, ab, msg_type);
+}

This makes sense because this is used by "user" records ...

kernel/audit.c static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) · line 1084

Quoted 4 lines

kernel/audit.c +4 -0

+static inline void audit_log_config_change_alt(struct audit_buffer **ab)
+{
+       audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
+}

... and I don't believe this makes sense because there is no real logical grouping with the callers like there is for audit_log_user_recv_msg().

paul moore

Quoted 37 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types, and since the AUDIT_CONFIG_CHANGE message type has been converted to a syscall accompanied record type, special-case the AUDIT_USER_* range of messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c          | 27 +++++++++++++++++++--------
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     |  2 +-
 kernel/audit_watch.c    |  2 +-
 kernel/auditfilter.c    |  2 +-
 5 files changed, 23 insertions(+), 12 deletions(-)

kernel/audit.c +0 -0

index 0e8026423fbd..a321fea94cc6 100644

kernel/audit.c static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) · line 1079

Quoted 7 lines

kernel/audit.c +4 -0

@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
        audit_log_task_context(*ab);
 }
 
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+       audit_log_common_recv_msg(NULL, ab, msg_type);
+}

This makes sense because this is used by "user" records ...

Quoted 8 lines

kernel/audit.c static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) · line 1084

Quoted 4 lines

kernel/audit.c +4 -0

+static inline void audit_log_config_change_alt(struct audit_buffer **ab)
+{
+       audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
+}

... and I don't believe this makes sense because there is no real logical grouping with the callers like there is for audit_log_user_recv_msg().

I don't follow "logical grouping". They are all CONFIG_CHANGE record prefixes with the current context.

Can you suggest an alternate name or another way of sharing audit_log_common_recv_msg() since the only differences between the two are a NULL context vs current task's context and the message type. I wasn't particularly happy with this name either. I'd really like to refactor this with all the rest of the CONFIG_CHANGE records, but there is too much of a format difference to make it work without reordering or deleting useless fields.

I know you had suggested making two different functions, but I think they are more similar than different and merit the common factored code.

Quoted 1 line

paul moore

- RGB

--
Richard Guy Briggs <[email protected]>
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

Quoted 3 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Quoted 24 lines

Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types, and since the AUDIT_CONFIG_CHANGE message type has been converted to a syscall accompanied record type, special-case the AUDIT_USER_* range of messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c          | 27 +++++++++++++++++++--------
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     |  2 +-
 kernel/audit_watch.c    |  2 +-
 kernel/auditfilter.c    |  2 +-
 5 files changed, 23 insertions(+), 12 deletions(-)

kernel/audit.c +0 -0

index 0e8026423fbd..a321fea94cc6 100644

kernel/audit.c static void audit_log_common_recv_msg(struct · line 1079

Quoted 7 lines

kernel/audit.c +3 -0

@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
 audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
 }
 
+static inline void audit_log_user_recv_msg(struct audit_buffer
 **ab, u16 msg_type) +{
+       audit_log_common_recv_msg(NULL, ab, msg_type);
+}  

This makes sense because this is used by "user" records ...

kernel/audit.c static void audit_log_common_recv_msg(struct · line 1083

Quoted 4 lines

kernel/audit.c +2 -0

+static inline void audit_log_config_change_alt(struct audit_buffer
 **ab) +{
+       audit_log_common_recv_msg(audit_context(), ab,
 AUDIT_CONFIG_CHANGE); +}  

... and I don't believe this makes sense because there is no real logical grouping with the callers like there is for audit_log_user_recv_msg().

Quoted 5 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 3 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

Quoted 11 lines · Steve Grubb

Quoted 3 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 2 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

We've discussed this quite a bit already; connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event. We've also heard from a prominent user that associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged.

paul moore

Quoted 26 lines · Paul Moore

Quoted 11 lines · Steve Grubb

Quoted 3 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 2 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

We've discussed this quite a bit already; connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event. We've also heard from a prominent user that associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Quoted 4 lines

Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged.

I would do the change, which should be very trivial, but I'm dense enough that I still don't know what you want. In the last 6 months I've asked a number of direct questions that have not been directly addressed. Perhaps I should be able to figure it out from the more general or fundamental principles replies I've gotten (which have been helpful, but perhaps incomplete), but I'm still having some trouble. Perhaps I'm exposing my limitations.

Quoted 33 lines · Richard Guy Briggs

Quoted 26 lines · Paul Moore

Quoted 11 lines · Steve Grubb

Quoted 3 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 2 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

We've discussed this quite a bit already; connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event. We've also heard from a prominent user that associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Quoted 4 lines

Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged.

Quoted 7 lines

I would do the change, which should be very trivial, but I'm dense enough that I still don't know what you want. In the last 6 months I've asked a number of direct questions that have not been directly addressed. Perhaps I should be able to figure it out from the more general or fundamental principles replies I've gotten (which have been helpful, but perhaps incomplete), but I'm still having some trouble. Perhaps I'm exposing my limitations.

Since code is unambiguous, let me just cut and paste what I was thinking (be warned, this is a cut-n-paste, so the whitespace is probably mangled):

kernel/audit.c +1 -1

index d412fb4ae6d5..d2caef6ef09e 100644
@@ -396,7 +396,7 @@ static int audit_log_config_change(char
 *function_name, u32 new, u32 old,
       struct audit_buffer *ab;
       int rc = 0;
 
-       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
       if (unlikely(!ab))
               return rc;

       audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);

kernel/audit.c +2 -1

@@ -1053,7 +1053,8 @@ static int audit_netlink_ok(struct sk_buff *skb,
 u16 msg_type)
       return err;
 }
 
-static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+static void audit_log_common_recv_msg(struct audit_context *context,
+                                       struct audit_buffer **ab, u16 msg_type)
 {
       uid_t uid = from_kuid(&init_user_ns, current_uid());

       pid_t pid = task_tgid_nr(current);

kernel/audit.c +1 -1

@@ -1063,7 +1064,7 @@ static void audit_log_common_recv_msg(struct
 audit_buffer **ab, u16 msg_type)
               return;
       }
 
-       *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+       *ab = audit_log_start(context, GFP_KERNEL, msg_type);
       if (unlikely(!*ab))
               return;

       audit_log_format(*ab, "pid=%d uid=%u ", pid, uid);

kernel/audit.c +5 -0

@@ -1071,6 +1072,11 @@ static void audit_log_common_recv_msg(struct
 audit_buffer **ab, u16 msg_type)
       audit_log_task_context(*ab);
 }
 
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
 u16 msg_type)
+{
+       audit_log_common_recv_msg(NULL, ab, msg_type);
+}
+
 int is_audit_feature_set(int i)

{
       return af.features & AUDIT_FEATURE_TO_MASK(i);

kernel/audit.c +1 -1

@@ -1338,7 +1344,7 @@ static int audit_receive_msg(struct sk_buff
 *skb, struct nlmsghdr *nlh)
                               if (err)
                                       break;
                       }
-                       audit_log_common_recv_msg(&ab, msg_type);
+                       audit_log_user_recv_msg(&ab, msg_type);
                       if (msg_type != AUDIT_USER_TTY)
                               audit_log_format(ab, " msg='%.*s'",

AUDIT_MESSAGE_TEXT_MAX,

kernel/audit.c +2 -1

@@ -1361,7 +1367,8 @@ static int audit_receive_msg(struct sk_buff
 *skb, struct nlmsghdr *nlh)
               if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
                       return -EINVAL;
               if (audit_enabled == AUDIT_LOCKED) {
-                       audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+                       audit_log_common_recv_msg(audit_context(), &ab,
+                                                 AUDIT_CONFIG_CHANGE);
                       audit_log_format(ab, " op=%s audit_enabled=%d res=0",
                                        msg_type == AUDIT_ADD_RULE ?

                                               "add_rule" : "remove_rule",

kernel/audit.c +2 -1

@@ -1376,7 +1383,8 @@ static int audit_receive_msg(struct sk_buff
 *skb, struct nlmsghdr *nlh)
               break;
       case AUDIT_TRIM:
               audit_trim_trees();
-               audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+               audit_log_common_recv_msg(audit_context(), &ab,
+                                         AUDIT_CONFIG_CHANGE);
               audit_log_format(ab, " op=trim res=1");
               audit_log_end(ab);

               break;

kernel/audit.c +2 -1

@@ -1406,7 +1414,8 @@ static int audit_receive_msg(struct sk_buff
 *skb, struct nlmsghdr *nlh)
               /* OK, here comes... */
               err = audit_tag_tree(old, new);
 
-               audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+               audit_log_common_recv_msg(audit_context(), &ab,
+                                         AUDIT_CONFIG_CHANGE);
 
               audit_log_format(ab, " op=make_equiv old=");

               audit_log_untrustedstring(ab, old);

kernel/audit.c +2 -1

@@ -1474,7 +1483,8 @@ static int audit_receive_msg(struct sk_buff
 *skb, struct nlmsghdr *nlh)
               old.enabled = t & AUDIT_TTY_ENABLE;
               old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
 
-               audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+               audit_log_common_recv_msg(audit_context(), &ab,
+                                         AUDIT_CONFIG_CHANGE);
               audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
                                " old-log_passwd=%d new-log_passwd=%d res=%d",

old.enabled, s.enabled, old.log_passwd,

kernel/audit_fsnotify.c +0 -0

index cf4512a33675..37ae95cfb7f4 100644
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct

audit_fsnotify_mark *audit_mark, c

       if (!audit_enabled)
               return;

kernel/audit_fsnotify.c +1 -1

-       ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+       ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
       if (unlikely(!ab))
               return;
       audit_log_session_info(ab);

kernel/audit_watch.c +0 -0

index 20ef9ba134b0..e8d1adeb2223 100644
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct

audit_krule *r, struct audit_watc

       if (!audit_enabled)
               return;

kernel/audit_watch.c +1 -1

-       ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+       ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
       if (!ab)
               return;
       audit_log_session_info(ab);

kernel/auditfilter.c +0 -0

index bf309f2592c4..26a80a9d43a9 100644
@@ -1091,7 +1091,7 @@ static void audit_log_rule_change(char *action,

struct audit_krule *rule, int re

       if (!audit_enabled)
               return;

kernel/auditfilter.c +1 -1

-       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
       if (!ab)
               return;
       audit_log_session_info(ab);

Signature · 3 lines

-- 
paul moore
www.paul-moore.com

Quoted 42 lines · Paul Moore

Quoted 40 lines · Richard Guy Briggs

Quoted 26 lines · Paul Moore

Quoted 11 lines · Steve Grubb

Quoted 3 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 2 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

We've discussed this quite a bit already; connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event. We've also heard from a prominent user that associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Quoted 4 lines

Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged.

I would do the change, which should be very trivial, but I'm dense enough that I still don't know what you want. In the last 6 months I've asked a number of direct questions that have not been directly addressed. Perhaps I should be able to figure it out from the more general or fundamental principles replies I've gotten (which have been helpful, but perhaps incomplete), but I'm still having some trouble. Perhaps I'm exposing my limitations.

Quoted 3 lines

Since code is unambiguous, let me just cut and paste what I was thinking (be warned, this is a cut-n-paste, so the whitespace is probably mangled):

Thank you. Very clear. I don't see the point of audit_log_user_recv_msg() for reasons already stated but that's ok.

Since we're looking at these, here are the various field formats of the AUDIT_CONFIG_CHANGE records.

Steve and Paul, are they worth attempting to normalize?

Quoted 43 lines · Richard Guy Briggs

Quoted 42 lines · Paul Moore

Quoted 40 lines · Richard Guy Briggs

Quoted 26 lines · Paul Moore

Quoted 11 lines · Steve Grubb

Quoted 3 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 2 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

We've discussed this quite a bit already; connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event. We've also heard from a prominent user that associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Quoted 4 lines

Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged.

I would do the change, which should be very trivial, but I'm dense enough that I still don't know what you want. In the last 6 months I've asked a number of direct questions that have not been directly addressed. Perhaps I should be able to figure it out from the more general or fundamental principles replies I've gotten (which have been helpful, but perhaps incomplete), but I'm still having some trouble. Perhaps I'm exposing my limitations.

Quoted 6 lines

Quoted 3 lines

Since code is unambiguous, let me just cut and paste what I was thinking (be warned, this is a cut-n-paste, so the whitespace is probably mangled):

Thank you. Very clear. I don't see the point of audit_log_user_recv_msg() for reasons already stated but that's ok.

That's a fair comment. I think there is some value in having the function dedicated for the user messages since they are fairly unique in that we don't ever want to associate them with the current task, but it does result in a single use function (which I generally dislike). Who knows, maybe at some future point in time we'll get rid of audit_log_user_recv_msg(), but let's stick with it for now.

Quoted 3 lines

Since we're looking at these, here are the various field formats of the AUDIT_CONFIG_CHANGE records.

Quoted 1 line

Steve and Paul, are they worth attempting to normalize?

Right now, my vote is "no". Although I'm voting that way pretty much just because I want to get this patch in during this development cycle and I'm fairly certain that trying to reach any consensus around normalization is going to drag this out. I would strongly encourage you to just tweak this patch as we've just talked about and leave it at that for the time being.

paul moore

Quoted 9 lines · Steve Grubb

Quoted 5 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 3 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Quoted 6 lines

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

We've discussed this quite a bit already;

connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event.

We've also heard from a prominent user that
associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged.

Quoted 17 lines · On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore <[email protected]> wrote:

Quoted 15 lines · Steve Grubb

Quoted 5 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 3 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

Quoted 1 line

We've discussed this quite a bit already;

Yes, and you still don't seem be listening. You have to cooperate on modifying events. We as a community need to respect each other's needs and work together to solve problems. What this is saying sounds a lot like I don't care if it breaks things, I'm gonna do it my way. Tough luck.

You do not have to make sense of any of these events. So, what does it really matter to you how the event is formed? What I'm asking for is have these changes been vetted to ensure that they are not breaking things?

Quoted 6 lines

connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event.

Except that they are not. The design of the audit system is to save disk space as much as possible by emitting single record events on certain event types that are simple. To add a syscall to it adds useless information (such as a socket address record), slows down processing, and wastes disk space. If you get a SYSCALL record, that indicates that you have triggered an event that the system admin has placed explicit rules on. That is different than the common criteria required configuration change event.

Quoted 7 lines

We've also heard from a prominent user that
associating records in this way is desirable.

If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future.

Isn't there some kind a guideline about not breaking user space?

Quoted 3 lines · Steve Grubb

Quoted 2 lines · On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore <[email protected]> wrote:

Quoted 1 line · Steve Grubb

On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines · Richard Guy Briggs

Quoted 2 lines

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

I see you added LKML to the To/CC line. Fun. Unless they've been following the audit list for the past several years I'm not sure they have all the context to fully understand some of the things in this thread, but sure, why not.

paul moore

Quoted 7 lines · Paul Moore

Quoted 6 lines · Steve Grubb

Quoted 5 lines · On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore <[email protected]> wrote:

Quoted 4 lines · Steve Grubb

Quoted 3 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 2 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Quoted 4 lines

I see you added LKML to the To/CC line. Fun. Unless they've been following the audit list for the past several years I'm not sure they have all the context to fully understand some of the things in this thread, but sure, why not.

I added LKML in the original patchset posting, not Steve.

Quoted 5 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 3 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

-Steve

Quoted 42 lines

Quoted 24 lines

Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types, and since the AUDIT_CONFIG_CHANGE message type has been converted to a syscall accompanied record type, special-case the AUDIT_USER_* range of messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c          | 27 +++++++++++++++++++--------
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     |  2 +-
 kernel/audit_watch.c    |  2 +-
 kernel/auditfilter.c    |  2 +-
 5 files changed, 23 insertions(+), 12 deletions(-)

kernel/audit.c +0 -0

index 0e8026423fbd..a321fea94cc6 100644

kernel/audit.c static void audit_log_common_recv_msg(struct · line 1079

Quoted 7 lines

kernel/audit.c +3 -0

@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
 audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
 }
 
+static inline void audit_log_user_recv_msg(struct audit_buffer
 **ab, u16 msg_type) +{
+       audit_log_common_recv_msg(NULL, ab, msg_type);
+}  

This makes sense because this is used by "user" records ...

kernel/audit.c static void audit_log_common_recv_msg(struct · line 1083

Quoted 4 lines

kernel/audit.c +2 -0

+static inline void audit_log_config_change_alt(struct audit_buffer
 **ab) +{
+       audit_log_common_recv_msg(audit_context(), ab,
 AUDIT_CONFIG_CHANGE); +}  

... and I don't believe this makes sense because there is no real logical grouping with the callers like there is for audit_log_user_recv_msg().

Quoted 9 lines · Steve Grubb

Quoted 5 lines · On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <[email protected]> wrote:

Quoted 3 lines · Richard Guy Briggs

Tie syscall information to all CONFIG_CHANGE calls since they are all a result of user actions.

Quoted 6 lines

Please don't tie syscall information to this. The syscall will be sendto. We don't need that information, its implicit. Also, doing this will possibly wreck things in libauparse. Please test the events with ausearch --format csv and --format text. IFF the event looks better or the same should we do this. If stuff disappears, the patch is breaking things

Steve, I hope you aren't talking about the AUDIT_*USER* records, because this patch intentionally leaves them unassociated with the syscall record.

The config change records are related.

Quoted 41 lines

-Steve

Quoted 39 lines

Quoted 24 lines

Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types, and since the AUDIT_CONFIG_CHANGE message type has been converted to a syscall accompanied record type, special-case the AUDIT_USER_* range of messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c          | 27 +++++++++++++++++++--------
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     |  2 +-
 kernel/audit_watch.c    |  2 +-
 kernel/auditfilter.c    |  2 +-
 5 files changed, 23 insertions(+), 12 deletions(-)

kernel/audit.c +0 -0

index 0e8026423fbd..a321fea94cc6 100644

kernel/audit.c static void audit_log_common_recv_msg(struct · line 1079

Quoted 7 lines

kernel/audit.c +3 -0

@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
 audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
 }
 
+static inline void audit_log_user_recv_msg(struct audit_buffer
 **ab, u16 msg_type) +{
+       audit_log_common_recv_msg(NULL, ab, msg_type);
+}  

This makes sense because this is used by "user" records ...

Quoted 4 lines

kernel/audit.c +2 -0

+static inline void audit_log_config_change_alt(struct audit_buffer
 **ab) +{
+       audit_log_common_recv_msg(audit_context(), ab,
 AUDIT_CONFIG_CHANGE); +}  

Quoted 3 lines

Quoted 3 lines

... and I don't believe this makes sense because there is no real logical grouping with the callers like there is for audit_log_user_recv_msg().

- RGB

Since the context is derived from the task parameter handed to __audit_free(), hand the context to audit_kill_trees() so it can be used to associate with a syscall record. This requires adding the context parameter to kill_rules() rather than using the current audit_context.

The callers of trim_marked() and evict_chunk() still have their context.

The EOE record was being issued prior to the pruning of the killed_tree list.

Move the kill_trees call before the audit_log_exit call in __audit_free() and __audit_syscall_exit() so that any pruned trees CONFIG_CHANGE records are included with the associated syscall event by the user library due to the EOE record flagging the end of the event.

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.h      |  4 ++--
 kernel/audit_tree.c | 18 ++++++++++--------
 kernel/auditsc.c    | 12 ++++++------
 3 files changed, 18 insertions(+), 16 deletions(-)

kernel/audit.h +2 -2

index 91421679a168..6ffb70575082 100644
@@ -314,7 +314,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 extern int audit_tag_tree(char *old, char *new);
 extern const char *audit_tree_path(struct audit_tree *tree);
 extern void audit_put_tree(struct audit_tree *tree);
-extern void audit_kill_trees(struct list_head *list);
+extern void audit_kill_trees(struct audit_context *context);
 #else
 #define audit_remove_tree_rule(rule) BUG()
 #define audit_add_tree_rule(rule) -EINVAL

@@ -323,7 +323,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 #define audit_put_tree(tree) (void)0
 #define audit_tag_tree(old, new) -EINVAL
 #define audit_tree_path(rule) ""       /* never called */
-#define audit_kill_trees(list) BUG()
+#define audit_kill_trees(context) BUG()
 #endif
 
 extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);

kernel/audit_tree.c +10 -8

index b0bd59ef4271..bf77d265e68e 100644
@@ -524,13 +524,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
        return 0;
 }
 
-static void audit_tree_log_remove_rule(struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
 {
        struct audit_buffer *ab;
 
        if (!audit_enabled)
                return;
-       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
        if (unlikely(!ab))
                return;
        audit_log_format(ab, "op=remove_rule dir=");

@@ -540,7 +540,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
        audit_log_end(ab);
 }
 
-static void kill_rules(struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree)
 {
        struct audit_krule *rule, *next;
        struct audit_entry *entry;

@@ -551,7 +551,7 @@ static void kill_rules(struct audit_tree *tree)
                list_del_init(&rule->rlist);
                if (rule->tree) {
                        /* not a half-baked one */
-                       audit_tree_log_remove_rule(rule);
+                       audit_tree_log_remove_rule(context, rule);
                        if (entry->rule.exe)
                                audit_remove_mark(entry->rule.exe);
                        rule->tree = NULL;

@@ -633,7 +633,7 @@ static void trim_marked(struct audit_tree *tree)
                tree->goner = 1;
                spin_unlock(&hash_lock);
                mutex_lock(&audit_filter_mutex);
-               kill_rules(tree);
+               kill_rules(audit_context(), tree);
                list_del_init(&tree->list);
                mutex_unlock(&audit_filter_mutex);
                prune_one(tree);

@@ -973,8 +973,10 @@ static void audit_schedule_prune(void)
  * ... and that one is done if evict_chunk() decides to delay until the end
  * of syscall.  Runs synchronously.
  */
-void audit_kill_trees(struct list_head *list)
+void audit_kill_trees(struct audit_context *context)
 {
+       struct list_head *list = &context->killed_trees;
+
        audit_ctl_lock();
        mutex_lock(&audit_filter_mutex);
 

@@ -982,7 +984,7 @@ void audit_kill_trees(struct list_head *list)
                struct audit_tree *victim;
 
                victim = list_entry(list->next, struct audit_tree, list);
-               kill_rules(victim);
+               kill_rules(context, victim);
                list_del_init(&victim->list);
 
                mutex_unlock(&audit_filter_mutex);

@@ -1017,7 +1019,7 @@ static void evict_chunk(struct audit_chunk *chunk)
                list_del_init(&owner->same_root);
                spin_unlock(&hash_lock);
                if (!postponed) {
-                       kill_rules(owner);
+                       kill_rules(audit_context(), owner);
                        list_move(&owner->list, &prune_list);
                        need_prune = 1;
                } else {

kernel/auditsc.c +6 -7

index 6593a5207fb0..b585ceb2f7a2 100644
@@ -1444,6 +1444,9 @@ void __audit_free(struct task_struct *tsk)
        if (!context)
                return;
 
+       if (!list_empty(&context->killed_trees))
+               audit_kill_trees(context);
+
        /* We are called either by do_exit() or the fork() error handling code;
         * in the former case tsk == current and in the latter tsk is a
         * random task_struct that doesn't doesn't have any meaningful data we

@@ -1460,9 +1463,6 @@ void __audit_free(struct task_struct *tsk)
                        audit_log_exit();
        }
 
-       if (!list_empty(&context->killed_trees))
-               audit_kill_trees(&context->killed_trees);
-
        audit_set_context(tsk, NULL);
        audit_free_context(context);
 }

@@ -1537,6 +1537,9 @@ void __audit_syscall_exit(int success, long return_code)
        if (!context)
                return;
 
+       if (!list_empty(&context->killed_trees))
+               audit_kill_trees(context);
+
        if (!context->dummy && context->in_syscall) {
                if (success)
                        context->return_valid = AUDITSC_SUCCESS;

@@ -1571,9 +1574,6 @@ void __audit_syscall_exit(int success, long return_code)
        context->in_syscall = 0;
        context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
 
-       if (!list_empty(&context->killed_trees))
-               audit_kill_trees(&context->killed_trees);
-
        audit_free_names(context);
        unroll_tree_refs(context, NULL, 0);
        audit_free_aux(context);
--

1.8.3.1

Give a clue as to the source of mark, watch and tree rule changes.

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     | 24 ++++++++++++------------
 kernel/audit_watch.c    |  6 ++++--
 kernel/auditsc.c        |  4 ++--
 5 files changed, 21 insertions(+), 19 deletions(-)

Make a number of changes to normalize CONFIG_CHANGE records by adding missing op= fields, providing more information in existing op fields (optional last patch) and connecting all records to existing audit events. The user record needs special-casing since its content isn't directly related to the call that logs it.

Since tree purge records are processed after the EOE record is produced, the order of operation of the EOE record and the purge will have to be reversed so that the purge records can be included in the event.

The last patch is included for completeness understanding it may be more information than necessary.

For reference, here are the calling methods and function tree for all CONFIG_CHANGE events with fields:
- audit_log_config_change()
- add "op=set" to fields: "[op] <param-name> old auid ses subj res"
- AUDIT_SET:AUDIT_STATUS_PID
- AUDIT_SET:AUDIT_STATUS_LOST
- audit_do_config_change()
- AUDIT_SET:AUDIT_STATUS_FAILURE
- AUDIT_SET:AUDIT_STATUS_ENABLED
- AUDIT_SET:AUDIT_STATUS_RATE_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_WAIT_TIME
- audit_log_rule_change()
- fields: "auid ses subj op key list res"
- AUDIT_ADD_RULE -F dir=...
- AUDIT_DEL_RULE -F dir=...
- audit_log_common_recv_msg()
- fields: "pid uid auid ses subj ..."
- AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
- AUDIT_LOCKED add "op={add,remove}_rule" to "[op] audit_enabled res"
- AUDIT_TRIM "op=trim res"
- AUDIT_MAKE_EQUIV: "op=make_equiv old new res"
- AUDIT_TTY_SET: "op=tty_set old-enabled new-enabled old-log_passwd new-log_passwd res"
- audit_mark_log_rule_change()
- add ":mark" to op in fields: "uid ses op=autoremove_rule[] path key list res"
- audit_autoremove_mark_rule()
- audit_mark_handle_event()
- audit_mark_fsnotify_ops.handle_event
- audit_tree_log_remove_rule() called from kill_rules()
- add to op ":tree:%s" to fields: "op=remove_rule[] dir key list res"
- from trim_marked()
- AUDIT_TRIM: audit_trim_trees() "trim"
- audit_add_tree_rule() iterate_mounts err "add"
- audit_add_rule()
- audit_rule_change()
- AUDIT_ADD_RULE -F dir=...
- AUDIT_MAKE_EQUIV: audit_tag_tree() iterate_mounts err "equiv"
- from audit_kill_trees()
- __audit_free() "free"
- do_exit()
- copy_process() err
- __audit_syscall_exit() "exit"
- from evict_chunk() "evict"
- audit_tree_freeing_mark()
- audit_tree_ops.freeing_mark
- audit_watch_log_rule_change()
add to op ":watch:%s" to fields "auid ses op={updated,remove}_rule[] path key list res"
- audit_update_watch() "updated_rules:watch:inval" : "updated_rules:watch:set"
- audit_watch_handle_event() FS_CREATE|FS_MOVED_TO, FS_DELETE|FS_MOVED_FROM
- audit_watch_fsnotify_ops.handle_event
- audit_remove_parent_watches() "remove_rule:watch:parent"
- audit_watch_handle_event() FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF
- audit_watch_fsnotify_ops.handle_event
- audit_seccomp_actions_logged()
- fields: "op actions old-actions res"

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59

Sources of AUDIT_CONFIG_CHANGE records and their current and proposed fields are listed here

        https://github.com/linux-audit/audit-kernel/issues/59#issuecomment-445055154

Changelog:
v3:
- un-clever %s_rule to not break up op values
- create audit_log_user_recv_msg() and squash into record connection
- squash kill_trees context handling with kill-trees before EOE
- rebase on audit/next (v4.20-rc1) with 2a1fe215e730 ("audit: use current whenever possible")
- remove parens in extended format

v2:
- re-order audit_log_exit() and audit_kill_trees()
- drop EOE reordering patch
- rebase on 4.18-rc1 (audit/next)

Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes

 kernel/audit.c          | 33 +++++++++++++++++++++++----------
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  4 ++--
 kernel/audit_tree.c     | 28 +++++++++++++++-------------
 kernel/audit_watch.c    |  8 +++++---
 kernel/auditfilter.c    |  2 +-
 kernel/auditsc.c        | 12 ++++++------
 7 files changed, 54 insertions(+), 37 deletions(-)

Quoted 96 lines · Richard Guy Briggs

Make a number of changes to normalize CONFIG_CHANGE records by adding missing op= fields, providing more information in existing op fields (optional last patch) and connecting all records to existing audit events. The user record needs special-casing since its content isn't directly related to the call that logs it.

Since tree purge records are processed after the EOE record is produced, the order of operation of the EOE record and the purge will have to be reversed so that the purge records can be included in the event.

The last patch is included for completeness understanding it may be more information than necessary.

For reference, here are the calling methods and function tree for all CONFIG_CHANGE events with fields:
- audit_log_config_change()
- add "op=set" to fields: "[op] <param-name> old auid ses subj res"
- AUDIT_SET:AUDIT_STATUS_PID
- AUDIT_SET:AUDIT_STATUS_LOST
- audit_do_config_change()
- AUDIT_SET:AUDIT_STATUS_FAILURE
- AUDIT_SET:AUDIT_STATUS_ENABLED
- AUDIT_SET:AUDIT_STATUS_RATE_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_WAIT_TIME
- audit_log_rule_change()
- fields: "auid ses subj op key list res"
- AUDIT_ADD_RULE -F dir=...
- AUDIT_DEL_RULE -F dir=...
- audit_log_common_recv_msg()
- fields: "pid uid auid ses subj ..."
- AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
- AUDIT_LOCKED add "op={add,remove}_rule" to "[op] audit_enabled res"
- AUDIT_TRIM "op=trim res"
- AUDIT_MAKE_EQUIV: "op=make_equiv old new res"
- AUDIT_TTY_SET: "op=tty_set old-enabled new-enabled old-log_passwd new-log_passwd res"
- audit_mark_log_rule_change()
- add ":mark" to op in fields: "uid ses op=autoremove_rule[] path key list res"
- audit_autoremove_mark_rule()
- audit_mark_handle_event()
- audit_mark_fsnotify_ops.handle_event
- audit_tree_log_remove_rule() called from kill_rules()
- add to op ":tree:%s" to fields: "op=remove_rule[] dir key list res"
- from trim_marked()
- AUDIT_TRIM: audit_trim_trees() "trim"
- audit_add_tree_rule() iterate_mounts err "add"
- audit_add_rule()
- audit_rule_change()
- AUDIT_ADD_RULE -F dir=...
- AUDIT_MAKE_EQUIV: audit_tag_tree() iterate_mounts err "equiv"
- from audit_kill_trees()
- __audit_free() "free"
- do_exit()
- copy_process() err
- __audit_syscall_exit() "exit"
- from evict_chunk() "evict"
- audit_tree_freeing_mark()
- audit_tree_ops.freeing_mark
- audit_watch_log_rule_change()
add to op ":watch:%s" to fields "auid ses op={updated,remove}_rule[] path key list res"
- audit_update_watch() "updated_rules:watch:inval" : "updated_rules:watch:set"
- audit_watch_handle_event() FS_CREATE|FS_MOVED_TO, FS_DELETE|FS_MOVED_FROM
- audit_watch_fsnotify_ops.handle_event
- audit_remove_parent_watches() "remove_rule:watch:parent"
- audit_watch_handle_event() FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF
- audit_watch_fsnotify_ops.handle_event
- audit_seccomp_actions_logged()
- fields: "op actions old-actions res"

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59

Sources of AUDIT_CONFIG_CHANGE records and their current and proposed fields are listed here

        https://github.com/linux-audit/audit-kernel/issues/59#issuecomment-445055154

Changelog:
v3:
- un-clever %s_rule to not break up op values
- create audit_log_user_recv_msg() and squash into record connection
- squash kill_trees context handling with kill-trees before EOE
- rebase on audit/next (v4.20-rc1) with 2a1fe215e730 ("audit: use current whenever possible")
- remove parens in extended format

v2:
- re-order audit_log_exit() and audit_kill_trees()
- drop EOE reordering patch
- rebase on 4.18-rc1 (audit/next)

Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes

Quoted 8 lines

 kernel/audit.c          | 33 +++++++++++++++++++++++----------
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  4 ++--
 kernel/audit_tree.c     | 28 +++++++++++++++-------------
 kernel/audit_watch.c    |  8 +++++---
 kernel/auditfilter.c    |  2 +-
 kernel/auditsc.c        | 12 ++++++------
 7 files changed, 54 insertions(+), 37 deletions(-)

In order to make sure expectations are set appropriately, as we are at -rc6 right now this is not something that would go into audit/next now (assuming everything looks okay on review), it would go into audit/next *after* the upcoming merge window.

paul moore

Quoted 1 line · Paul Moore

On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <[email protected]> wrote:

Quoted 15 lines

Quoted 6 lines

Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes

Quoted 8 lines

 kernel/audit.c          | 33 +++++++++++++++++++++++----------
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  4 ++--
 kernel/audit_tree.c     | 28 +++++++++++++++-------------
 kernel/audit_watch.c    |  8 +++++---
 kernel/auditfilter.c    |  2 +-
 kernel/auditsc.c        | 12 ++++++------
 7 files changed, 54 insertions(+), 37 deletions(-)

Quoted 4 lines

In order to make sure expectations are set appropriately, as we are at -rc6 right now this is not something that would go into audit/next now (assuming everything looks okay on review), it would go into audit/next *after* the upcoming merge window.

I agree it is a bit late for this. I wasn't expecting it to go in this one. I'm filling the queue since I'm blocked on other review for ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another merge window.

Quoted 2 lines · Richard Guy Briggs

Quoted 1 line · Paul Moore

On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <[email protected]> wrote:

...

Quoted 20 lines

Quoted 15 lines

Quoted 6 lines

Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes

Quoted 8 lines

 kernel/audit.c          | 33 +++++++++++++++++++++++----------
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  4 ++--
 kernel/audit_tree.c     | 28 +++++++++++++++-------------
 kernel/audit_watch.c    |  8 +++++---
 kernel/auditfilter.c    |  2 +-
 kernel/auditsc.c        | 12 ++++++------
 7 files changed, 54 insertions(+), 37 deletions(-)

Quoted 4 lines

In order to make sure expectations are set appropriately, as we are at -rc6 right now this is not something that would go into audit/next now (assuming everything looks okay on review), it would go into audit/next *after* the upcoming merge window.

Quoted 4 lines

I agree it is a bit late for this. I wasn't expecting it to go in this one. I'm filling the queue since I'm blocked on other review for ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another merge window.

As discussed previously, GHAK81
(https://github.com/linux-audit/audit-kernel/issues/81) is something that I consider part of the audit container ID work (GHAK90). I believe it's time to stop treating it as a separate issue.

The audit container ID work, GHAK90
(https://github.com/linux-audit/audit-kernel/issues/90), is where all the dragon's lie. That one takes a good deal of time to review, and quite frankly I'm really the only one who seems to be looking at it anymore, so it takes a bit longer.

Beside the fact that GHAK100
(https://github.com/linux-audit/audit-kernel/issues/100) was marked as a RFC, I've been waiting to hear back from the VFS folks if they are comfortable with it. Miklos Szeredi in particular had some concerns and it isn't clear to me from that thread that his concerns have been resolved.

paul moore

Quoted 31 lines · Paul Moore

Quoted 2 lines · Richard Guy Briggs

Quoted 1 line · Paul Moore

On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <[email protected]> wrote:

...

Quoted 24 lines

Quoted 15 lines

Quoted 6 lines

Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes

Quoted 8 lines

 kernel/audit.c          | 33 +++++++++++++++++++++++----------
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  4 ++--
 kernel/audit_tree.c     | 28 +++++++++++++++-------------
 kernel/audit_watch.c    |  8 +++++---
 kernel/auditfilter.c    |  2 +-
 kernel/auditsc.c        | 12 ++++++------
 7 files changed, 54 insertions(+), 37 deletions(-)

Quoted 4 lines

In order to make sure expectations are set appropriately, as we are at -rc6 right now this is not something that would go into audit/next now (assuming everything looks okay on review), it would go into audit/next *after* the upcoming merge window.

I agree it is a bit late for this. I wasn't expecting it to go in this one. I'm filling the queue since I'm blocked on other review for ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another merge window.

Quoted 4 lines

As discussed previously, GHAK81
(https://github.com/linux-audit/audit-kernel/issues/81) is something that I consider part of the audit container ID work (GHAK90). I believe it's time to stop treating it as a separate issue.

Fine by me. It was included in the ghak90 patchset this time and still is in v5, waiting to get the questions replied to that arose out of the review of v4 around Hallowe'en.