[RFC PATCH ghak59 V1 0/6] audit: config_change normalizations and event record gathering

[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 and
connecting all records to existing audit events.

The user record patch is included but is *optional* since there is doubt
that we want to disconnect the records from a single event.

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.

Could I get some feedback on the format of the op field values
themselves? They shouldn't cause any text processing headaches but
there may be a better way of expressing them.

For reference, here are the calling methods and function tree for all
CONFIG_CHANGE events:
- audit_log_config_change() "op=set"
- 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_common_recv_msg()
- AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
- AUDIT_LOCKED "op=%s_rule"(add/remove)
- AUDIT_TRIM "op=trim"
- AUDIT_MAKE_EQUIV: "op=make_equiv"
- AUDIT_TTY_SET: "op=tty_set"
- audit_log_rule_change()
- AUDIT_ADD_RULE -F dir=:
- AUDIT_DEL_RULE -F dir=:
- audit_mark_log_rule_change()
- audit_autoremove_mark_rule() "op=autoremove_rule(mark)"
- audit_mark_handle_event()
- audit_mark_fsnotify_ops.handle_event
- audit_tree_log_remove_rule() "op=remove_rule(tree:%s)" from kill_rules()
- 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()
- 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

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

Richard Guy Briggs (6):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: exclude user records from syscall context
audit: hand taken context to audit_kill_trees for syscall logging
audit: move EOE record after kill_trees for exit/free
audit: extend config_change mark/watch/tree rule changes

kernel/audit.c | 20 ++++++++++++++------
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 | 26 ++++++++++++++++++--------
7 files changed, 57 insertions(+), 35 deletions(-)

[Richard Guy Briggs]

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 <***@redhat.com>
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}

[Paul Moore]

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.

With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

There appears to be another one in audit_mark_log_rule_change() ...
and one more in audit_receive_msg(). There may be more.

Post by Richard Guy Briggs
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
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
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.
With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

The version I'm looking at already had it when it was added in 2009.

This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

Post by Paul Moore
There appears to be another one in audit_mark_log_rule_change() ...

Same, 2015.

Post by Paul Moore
and one more in audit_receive_msg(). There may be more.

I believe they're covered by other patches in the ghak59 set.

Post by Paul Moore

Post by Richard Guy Briggs
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
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
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.
With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

The version I'm looking at already had it when it was added in 2009.

Yup, there it is ... now I'm wondering what tree I was looking at as a
reference while reviewing this?

/me scratches head

Post by Richard Guy Briggs
This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

Yeah, I'm not concerned about that for the reasons you state.

Post by Richard Guy Briggs

Post by Paul Moore
and one more in audit_receive_msg(). There may be more.

I believe they're covered by other patches in the ghak59 set.

If they are in the later patches it might be good to move those "op="
additions into this patch.

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
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
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1

--
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.
With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

The version I'm looking at already had it when it was added in 2009.

Yup, there it is ... now I'm wondering what tree I was looking at as a
reference while reviewing this?
/me scratches head

Post by Richard Guy Briggs
This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

Yeah, I'm not concerned about that for the reasons you state.

Post by Richard Guy Briggs

Post by Paul Moore
and one more in audit_receive_msg(). There may be more.

I believe they're covered by other patches in the ghak59 set.

If they are in the later patches it might be good to move those "op="
additions into this patch.

I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
that are missing op= field. Can you narrow it down?

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
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
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1

--
paul moore
www.paul-moore.com

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

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.
With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

The version I'm looking at already had it when it was added in 2009.

Yup, there it is ... now I'm wondering what tree I was looking at as a
reference while reviewing this?
/me scratches head

Post by Richard Guy Briggs
This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

Yeah, I'm not concerned about that for the reasons you state.

Post by Richard Guy Briggs

Post by Paul Moore
and one more in audit_receive_msg(). There may be more.

I believe they're covered by other patches in the ghak59 set.

If they are in the later patches it might be good to move those "op="
additions into this patch.

I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
that are missing op= field. Can you narrow it down?

Well, just grep'ing my way through audit_receive_msg() I see that
AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
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
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1

--
paul moore
www.paul-moore.com

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

--
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.
With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

The version I'm looking at already had it when it was added in 2009.

Yup, there it is ... now I'm wondering what tree I was looking at as a
reference while reviewing this?
/me scratches head

Post by Richard Guy Briggs
This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

Yeah, I'm not concerned about that for the reasons you state.

Post by Richard Guy Briggs

Post by Paul Moore
and one more in audit_receive_msg(). There may be more.

I believe they're covered by other patches in the ghak59 set.

If they are in the later patches it might be good to move those "op="
additions into this patch.

I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
that are missing op= field. Can you narrow it down?

Well, just grep'ing my way through audit_receive_msg() I see that
AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.

The failure case is addressed in this patch. The success case is
addressed in audit_log_rule_change(). The latter already has it. What
is the problem? What tree are you looking at? What am I missing?

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
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
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,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)
@@ -1365,7 +1365,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_rule audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1

--
paul moore
www.paul-moore.com

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

--
paul moore
www.paul-moore.com

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

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.
With that out of the way, I think this patch is fine, but I don't
think it is complete. At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field. If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

The version I'm looking at already had it when it was added in 2009.

Yup, there it is ... now I'm wondering what tree I was looking at as a
reference while reviewing this?
/me scratches head

Post by Richard Guy Briggs
This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

Yeah, I'm not concerned about that for the reasons you state.

Post by Richard Guy Briggs

Post by Paul Moore
and one more in audit_receive_msg(). There may be more.

I believe they're covered by other patches in the ghak59 set.

If they are in the later patches it might be good to move those "op="
additions into this patch.

I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
that are missing op= field. Can you narrow it down?

Well, just grep'ing my way through audit_receive_msg() I see that
AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.

The failure case is addressed in this patch. The success case is
addressed in audit_log_rule_change(). The latter already has it. What
is the problem? What tree are you looking at? What am I missing?

So it does. This discussion dragged out long enough that I forgot to
check the actual patch submission.

I think this patch is fine, I would recommend updating this patchset
using the feedback on the other individual patches and resubmitting.

[Richard Guy Briggs]

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

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 <***@redhat.com>
---
kernel/audit.c | 4 ++--
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 2 +-
kernel/audit_watch.c | 2 +-
kernel/auditfilter.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index ad54339..e469234 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,7 +400,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);
@@ -1067,7 +1067,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(audit_context(), GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 52f368b..1640eb6 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c

if (!audit_enabled)
return;
- 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_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 5e9d1e5..a01b9da 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -499,7 +499,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)

if (!audit_enabled)
return;
- 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;
audit_log_format(ab, "op=remove_rule");
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 9b4836b..da2978b 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc

if (!audit_enabled)
return;
- 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_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index eaa3201..6e19acb 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1093,7 +1093,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
if (!audit_enabled)
return;

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

[Paul Moore]

Post by Richard Guy Briggs
Tie syscall information to all CONFIG_CHANGE calls since they are all a
result of user actions.
See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50
---
kernel/audit.c | 4 ++--
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 2 +-
kernel/audit_watch.c | 2 +-
kernel/auditfilter.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)

Merged, thanks.

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index ad54339..e469234 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,7 +400,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);
@@ -1067,7 +1067,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(audit_context(), GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 52f368b..1640eb6 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
if (!audit_enabled)
return;
- 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_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 5e9d1e5..a01b9da 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -499,7 +499,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
if (!audit_enabled)
return;
- 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;
audit_log_format(ab, "op=remove_rule");
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 9b4836b..da2978b 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc
if (!audit_enabled)
return;
- 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_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index eaa3201..6e19acb 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1093,7 +1093,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
if (!audit_enabled)
return;
- 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);
--
1.8.3.1

[Paul Moore]

Post by Paul Moore

Post by Richard Guy Briggs
Tie syscall information to all CONFIG_CHANGE calls since they are all a
result of user actions.
See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50
---
kernel/audit.c | 4 ++--
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 2 +-
kernel/audit_watch.c | 2 +-
kernel/auditfilter.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)

Merged, thanks.

Actually, I take that back (good thing I hadn't pushed yet).

Why don't you squash this patch with 3/6 so that a bisect or
cherry-pick backport doesn't end up splitting 2/6 and 3/6 and causing
a regression with the AUDIT_USER_* records.

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index ad54339..e469234 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,7 +400,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);
@@ -1067,7 +1067,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(audit_context(), GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 52f368b..1640eb6 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
if (!audit_enabled)
return;
- 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_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 5e9d1e5..a01b9da 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -499,7 +499,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
if (!audit_enabled)
return;
- 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;
audit_log_format(ab, "op=remove_rule");
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 9b4836b..da2978b 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc
if (!audit_enabled)
return;
- 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_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index eaa3201..6e19acb 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1093,7 +1093,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
if (!audit_enabled)
return;
- 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);
--
1.8.3.1

--
paul moore
www.paul-moore.com

[Richard Guy Briggs]

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
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,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);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}

- *ab = audit_log_start(audit_context(), 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);
@@ -1075,6 +1076,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_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ __audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,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_common_recv_msg(NULL, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,

[Paul Moore]

Post by Richard Guy Briggs
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
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,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);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(audit_context(), 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);
@@ -1075,6 +1076,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_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ __audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,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_common_recv_msg(NULL, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
--
1.8.3.1

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
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
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

This one I left as a seperate patch for discussion. We'd previously
talked about connecting all possible records with syscall records if
they exist, but this one I'm unsure about, since we don't really care
what userspace process is issuing this message. It is just the message
content itself that is important. Or is it? Are we concerned about
CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
can get in case they go rogue or pear-shaped?

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,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);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(audit_context(), 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);
@@ -1075,6 +1076,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_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ __audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,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_common_recv_msg(NULL, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
--
1.8.3.1

--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <***@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

[Richard Guy Briggs]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

This one I left as a seperate patch for discussion. We'd previously
talked about connecting all possible records with syscall records if
they exist, but this one I'm unsure about, since we don't really care
what userspace process is issuing this message. It is just the message
content itself that is important. Or is it? Are we concerned about
CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
can get in case they go rogue or pear-shaped?

I'm waiting on re-spinning this patchset because of this open question.

Is connecting AUDIT_USER* records desirable or a liability?

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,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);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(audit_context(), 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);
@@ -1075,6 +1076,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_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ __audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,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_common_recv_msg(NULL, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
--
1.8.3.1

--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB
--
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
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <***@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

[Paul Moore]

Post by Richard Guy Briggs

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

This one I left as a seperate patch for discussion. We'd previously
talked about connecting all possible records with syscall records if
they exist, but this one I'm unsure about, since we don't really care
what userspace process is issuing this message. It is just the message
content itself that is important. Or is it? Are we concerned about
CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
can get in case they go rogue or pear-shaped?

I'm waiting on re-spinning this patchset because of this open question.
Is connecting AUDIT_USER* records desirable or a liability?

Like I said, I think special casing the AUDIT_USER* records so they
are *not* associated with other records is okay, and perhaps even the
right thing to do. The problem is that we don't have the necessary
context (har har) to match any kernel events (and there is the
possibility that there are none) to the userspace generated
AUDIT_USER* event. Further, I don't think this is something we would
ever be able to solve in a reasonable manner.

Post by Richard Guy Briggs

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,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);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(audit_context(), 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);
@@ -1075,6 +1076,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_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ __audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,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_common_recv_msg(NULL, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
--
1.8.3.1

--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB
--
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
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

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

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

This one I left as a seperate patch for discussion. We'd previously
talked about connecting all possible records with syscall records if
they exist, but this one I'm unsure about, since we don't really care
what userspace process is issuing this message. It is just the message
content itself that is important. Or is it? Are we concerned about
CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
can get in case they go rogue or pear-shaped?

I'm waiting on re-spinning this patchset because of this open question.
Is connecting AUDIT_USER* records desirable or a liability?

Like I said, I think special casing the AUDIT_USER* records so they
are *not* associated with other records is okay, and perhaps even the
right thing to do. The problem is that we don't have the necessary
context (har har) to match any kernel events (and there is the
possibility that there are none) to the userspace generated
AUDIT_USER* event. Further, I don't think this is something we would
ever be able to solve in a reasonable manner.

Ok, having said that, I think I'd still prefer to keep this patch
seperate, partly to retain the simplicity of the previous patch and make
very clear what each one is doing, and partly if we decide to change our
mind in the future that these AUDIT_USER* records should be autonomous.

Post by Paul Moore

Post by Richard Guy Briggs

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,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);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(audit_context(), 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);
@@ -1075,6 +1076,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_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ __audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,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_common_recv_msg(NULL, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
--
1.8.3.1

--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB
--
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
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

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

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
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
---
kernel/audit.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

This one I left as a seperate patch for discussion. We'd previously
talked about connecting all possible records with syscall records if
they exist, but this one I'm unsure about, since we don't really care
what userspace process is issuing this message. It is just the message
content itself that is important. Or is it? Are we concerned about
CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
can get in case they go rogue or pear-shaped?

I'm waiting on re-spinning this patchset because of this open question.
Is connecting AUDIT_USER* records desirable or a liability?

Like I said, I think special casing the AUDIT_USER* records so they
are *not* associated with other records is okay, and perhaps even the
right thing to do. The problem is that we don't have the necessary
context (har har) to match any kernel events (and there is the
possibility that there are none) to the userspace generated
AUDIT_USER* event. Further, I don't think this is something we would
ever be able to solve in a reasonable manner.

Ok, having said that, I think I'd still prefer to keep this patch
seperate, partly to retain the simplicity of the previous patch and make
very clear what each one is doing, and partly if we decide to change our
mind in the future that these AUDIT_USER* records should be autonomous.

Okay, I'll buy that argument.

[Richard Guy Briggs]

Since the context is taken from the task in __audit_syscall_exit() and
__audit_free(), hand it 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
(which has been taken).

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

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 <***@redhat.com>
---
kernel/audit.h | 4 ++--
kernel/audit_tree.c | 18 ++++++++++--------
kernel/auditsc.c | 4 ++--
3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index 214e149..f39f7aa 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,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
@@ -321,7 +321,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);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index a01b9da..2d3e1071 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,13 +493,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");
@@ -510,7 +510,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;
@@ -521,7 +521,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;
@@ -584,7 +584,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);
@@ -924,8 +924,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);

@@ -933,7 +935,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);
@@ -972,7 +974,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 {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c45..2590c9e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1490,7 +1490,7 @@ void __audit_free(struct task_struct *tsk)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
+ audit_kill_trees(context);

audit_free_context(context);
}
@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;

if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
+ audit_kill_trees(context);

audit_free_names(context);
unroll_tree_refs(context, NULL, 0);

[Paul Moore]

Post by Richard Guy Briggs
Since the context is taken from the task in __audit_syscall_exit() and
__audit_free(), hand it 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
(which has been taken).
The callers of trim_marked() and evict_chunk() still have their context.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
---
kernel/audit.h | 4 ++--
kernel/audit_tree.c | 18 ++++++++++--------
kernel/auditsc.c | 4 ++--
3 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index 214e149..f39f7aa 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,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
@@ -321,7 +321,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);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index a01b9da..2d3e1071 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,13 +493,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");
@@ -510,7 +510,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;
@@ -521,7 +521,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;
@@ -584,7 +584,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);
@@ -924,8 +924,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);
@@ -933,7 +935,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);
@@ -972,7 +974,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 {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c45..2590c9e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1490,7 +1490,7 @@ void __audit_free(struct task_struct *tsk)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
+ audit_kill_trees(context);

See my comment below about the ordering of audit_kill_trees() and
audit_log_exit().

Post by Richard Guy Briggs
audit_free_context(context);
}
@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
+ audit_kill_trees(context);

I wonder if we should move the kill_trees if-block above the
audit_log_exit() block so that any records that are emitted will be
before the SYSCALL record. I didn't chase down all the code paths,
but it seems like it should be safe, no?

Post by Richard Guy Briggs
audit_free_names(context);
unroll_tree_refs(context, NULL, 0);
--
1.8.3.1

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
Since the context is taken from the task in __audit_syscall_exit() and
__audit_free(), hand it 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
(which has been taken).
The callers of trim_marked() and evict_chunk() still have their context.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
---
kernel/audit.h | 4 ++--
kernel/audit_tree.c | 18 ++++++++++--------
kernel/auditsc.c | 4 ++--
3 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index 214e149..f39f7aa 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,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
@@ -321,7 +321,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);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index a01b9da..2d3e1071 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,13 +493,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");
@@ -510,7 +510,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;
@@ -521,7 +521,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;
@@ -584,7 +584,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);
@@ -924,8 +924,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);
@@ -933,7 +935,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);
@@ -972,7 +974,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 {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c45..2590c9e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1490,7 +1490,7 @@ void __audit_free(struct task_struct *tsk)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
+ audit_kill_trees(context);

See my comment below about the ordering of audit_kill_trees() and
audit_log_exit().

Post by Richard Guy Briggs
audit_free_context(context);
}
@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
+ audit_kill_trees(context);

I wonder if we should move the kill_trees if-block above the
audit_log_exit() block so that any records that are emitted will be
before the SYSCALL record. I didn't chase down all the code paths,
but it seems like it should be safe, no?

Interesting. I thought I had looked at re-ordering them and rejected
that approach due to that information being needed for audit_log_exit(),
but I don't find any such dependency this pass through the code.

I guess the only concern I have then is that if the state is anything
other than AUDIT_RECORD_CONTEXT it would be an orphan record, but it
would be regardless with the existing code or with my proposed changes.
Perhaps that is a bug to start with, though I'm not sure it is at all
serious, so I'm not concerned about it.

I think re-ordering should be safe and that eliminates the seeming
complexity introduced by the next patch, which is a good thing.

Post by Paul Moore

Post by Richard Guy Briggs
audit_free_names(context);
unroll_tree_refs(context, NULL, 0);

paul moore

- RGB

--
Richard Guy Briggs <***@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

[Richard Guy Briggs]

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 <***@redhat.com>
---
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(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index f39f7aa..5e072f5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,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 audit_context *context);
+extern void audit_kill_trees(struct audit_context *context, char *trig);
#else
#define audit_remove_tree_rule(rule) BUG()
#define audit_add_tree_rule(rule) -EINVAL
@@ -321,7 +321,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(context) BUG()
+#define audit_kill_trees(context, trig) BUG()
#endif

extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 1640eb6..c10ba91 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -158,7 +158,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
struct audit_krule *rule = audit_mark->rule;
struct audit_entry *entry = container_of(rule, struct audit_entry, rule);

- audit_mark_log_rule_change(audit_mark, "autoremove_rule");
+ audit_mark_log_rule_change(audit_mark, "autoremove_rule(mark)");
audit_del_rule(entry);
}

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 2d3e1071..1726cfa 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,7 +493,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
return 0;
}

-static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule, char *trig)
{
struct audit_buffer *ab;

@@ -502,7 +502,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
- audit_log_format(ab, "op=remove_rule");
+ audit_log_format(ab, "op=remove_rule(tree:%s)", trig);
audit_log_format(ab, " dir=");
audit_log_untrustedstring(ab, rule->tree->pathname);
audit_log_key(ab, rule->filterkey);
@@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
audit_log_end(ab);
}

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

/* trim the uncommitted chunks from tree */

-static void trim_marked(struct audit_tree *tree)
+static void trim_marked(struct audit_tree *tree, char *trig)
{
struct list_head *p, *q;
spin_lock(&hash_lock);
@@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
tree->goner = 1;
spin_unlock(&hash_lock);
mutex_lock(&audit_filter_mutex);
- kill_rules(audit_context(), tree);
+ kill_rules(audit_context(), tree, trig);
list_del_init(&tree->list);
mutex_unlock(&audit_filter_mutex);
prune_one(tree);
@@ -665,7 +665,7 @@ void audit_trim_trees(void)
node->index &= ~(1U<<31);
}
spin_unlock(&hash_lock);
- trim_marked(tree);
+ trim_marked(tree, "trim");
drop_collected_mounts(root_mnt);
skip_it:
put_tree(tree);
@@ -798,7 +798,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
node->index &= ~(1U<<31);
spin_unlock(&hash_lock);
} else {
- trim_marked(tree);
+ trim_marked(tree, "add");
goto Err;
}

@@ -900,7 +900,7 @@ int audit_tag_tree(char *old, char *new)
node->index &= ~(1U<<31);
spin_unlock(&hash_lock);
} else {
- trim_marked(tree);
+ trim_marked(tree, "equiv");
}

put_tree(tree);
@@ -924,7 +924,7 @@ 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 audit_context *context)
+void audit_kill_trees(struct audit_context *context, char *trig)
{
struct list_head *list = &context->killed_trees;

@@ -935,7 +935,7 @@ void audit_kill_trees(struct audit_context *context)
struct audit_tree *victim;

victim = list_entry(list->next, struct audit_tree, list);
- kill_rules(context, victim);
+ kill_rules(context, victim, trig);
list_del_init(&victim->list);

mutex_unlock(&audit_filter_mutex);
@@ -974,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
list_del_init(&owner->same_root);
spin_unlock(&hash_lock);
if (!postponed) {
- kill_rules(audit_context(), owner);
+ kill_rules(audit_context(), owner, "evict");
list_move(&owner->list, &prune_list);
need_prune = 1;
} else {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index da2978b..693d0a8 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -317,7 +317,9 @@ static void audit_update_watch(struct audit_parent *parent,
if (oentry->rule.exe)
audit_remove_mark(oentry->rule.exe);

- audit_watch_log_rule_change(r, owatch, "updated_rules");
+ audit_watch_log_rule_change(r, owatch, invalidating ?
+ "updated_rules(watch:inval)" :
+ "updated_rules(watch:set)");

call_rcu(&oentry->rcu, audit_free_rule_rcu);
}
@@ -345,7 +347,7 @@ static void audit_remove_parent_watches(struct audit_parent *parent)
list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
e = container_of(r, struct audit_entry, rule);
- audit_watch_log_rule_change(r, w, "remove_rule");
+ audit_watch_log_rule_change(r, w, "remove_rule(watch:parent)");
if (e->rule.exe)
audit_remove_mark(e->rule.exe);
list_del(&r->rlist);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d56aead..32428a3 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1486,7 +1486,7 @@ void __audit_free(struct task_struct *tsk)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
+ audit_kill_trees(context, "free");
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
struct audit_buffer *ab;

@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, current);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
+ audit_kill_trees(context, "exit");
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
struct audit_buffer *ab;

[Paul Moore]

Post by Richard Guy Briggs
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
---
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(-)

I think having some additional context here would be helpful for
everyone, so I agree with this on principle. However, I think we need
to get clarification from Steve that his parser is able to handle
these richer "op" values.

Post by Richard Guy Briggs
diff --git a/kernel/audit.h b/kernel/audit.h
index f39f7aa..5e072f5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,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 audit_context *context);
+extern void audit_kill_trees(struct audit_context *context, char *trig);
#else
#define audit_remove_tree_rule(rule) BUG()
#define audit_add_tree_rule(rule) -EINVAL
@@ -321,7 +321,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(context) BUG()
+#define audit_kill_trees(context, trig) BUG()
#endif
extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 1640eb6..c10ba91 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -158,7 +158,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
struct audit_krule *rule = audit_mark->rule;
struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
- audit_mark_log_rule_change(audit_mark, "autoremove_rule");
+ audit_mark_log_rule_change(audit_mark, "autoremove_rule(mark)");
audit_del_rule(entry);
}
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 2d3e1071..1726cfa 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,7 +493,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
return 0;
}
-static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule, char *trig)
{
struct audit_buffer *ab;
@@ -502,7 +502,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
- audit_log_format(ab, "op=remove_rule");
+ audit_log_format(ab, "op=remove_rule(tree:%s)", trig);
audit_log_format(ab, " dir=");
audit_log_untrustedstring(ab, rule->tree->pathname);
audit_log_key(ab, rule->filterkey);
@@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
audit_log_end(ab);
}
-static void kill_rules(struct audit_context *context, struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree, char *trig)
{
struct audit_krule *rule, *next;
struct audit_entry *entry;
@@ -521,7 +521,7 @@ static void kill_rules(struct audit_context *context, struct audit_tree *tree)
list_del_init(&rule->rlist);
if (rule->tree) {
/* not a half-baked one */
- audit_tree_log_remove_rule(context, rule);
+ audit_tree_log_remove_rule(context, rule, trig);
if (entry->rule.exe)
audit_remove_mark(entry->rule.exe);
rule->tree = NULL;
@@ -551,7 +551,7 @@ static void prune_one(struct audit_tree *victim)
/* trim the uncommitted chunks from tree */
-static void trim_marked(struct audit_tree *tree)
+static void trim_marked(struct audit_tree *tree, char *trig)
{
struct list_head *p, *q;
spin_lock(&hash_lock);
@@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
tree->goner = 1;
spin_unlock(&hash_lock);
mutex_lock(&audit_filter_mutex);
- kill_rules(audit_context(), tree);
+ kill_rules(audit_context(), tree, trig);
list_del_init(&tree->list);
mutex_unlock(&audit_filter_mutex);
prune_one(tree);
@@ -665,7 +665,7 @@ void audit_trim_trees(void)
node->index &= ~(1U<<31);
}
spin_unlock(&hash_lock);
- trim_marked(tree);
+ trim_marked(tree, "trim");
drop_collected_mounts(root_mnt);
put_tree(tree);
@@ -798,7 +798,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
node->index &= ~(1U<<31);
spin_unlock(&hash_lock);
} else {
- trim_marked(tree);
+ trim_marked(tree, "add");
goto Err;
}
@@ -900,7 +900,7 @@ int audit_tag_tree(char *old, char *new)
node->index &= ~(1U<<31);
spin_unlock(&hash_lock);
} else {
- trim_marked(tree);
+ trim_marked(tree, "equiv");
}
put_tree(tree);
@@ -924,7 +924,7 @@ 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 audit_context *context)
+void audit_kill_trees(struct audit_context *context, char *trig)
{
struct list_head *list = &context->killed_trees;
@@ -935,7 +935,7 @@ void audit_kill_trees(struct audit_context *context)
struct audit_tree *victim;
victim = list_entry(list->next, struct audit_tree, list);
- kill_rules(context, victim);
+ kill_rules(context, victim, trig);
list_del_init(&victim->list);
mutex_unlock(&audit_filter_mutex);
@@ -974,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
list_del_init(&owner->same_root);
spin_unlock(&hash_lock);
if (!postponed) {
- kill_rules(audit_context(), owner);
+ kill_rules(audit_context(), owner, "evict");
list_move(&owner->list, &prune_list);
need_prune = 1;
} else {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index da2978b..693d0a8 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -317,7 +317,9 @@ static void audit_update_watch(struct audit_parent *parent,
if (oentry->rule.exe)
audit_remove_mark(oentry->rule.exe);
- audit_watch_log_rule_change(r, owatch, "updated_rules");
+ audit_watch_log_rule_change(r, owatch, invalidating ?
+ "updated_rules(watch:set)");
call_rcu(&oentry->rcu, audit_free_rule_rcu);
}
@@ -345,7 +347,7 @@ static void audit_remove_parent_watches(struct audit_parent *parent)
list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
e = container_of(r, struct audit_entry, rule);
- audit_watch_log_rule_change(r, w, "remove_rule");
+ audit_watch_log_rule_change(r, w, "remove_rule(watch:parent)");
if (e->rule.exe)
audit_remove_mark(e->rule.exe);
list_del(&r->rlist);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d56aead..32428a3 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1486,7 +1486,7 @@ void __audit_free(struct task_struct *tsk)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
+ audit_kill_trees(context, "free");
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
struct audit_buffer *ab;
@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, current);
if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
+ audit_kill_trees(context, "exit");
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
struct audit_buffer *ab;
--
1.8.3.1

[Steve Grubb]

Post by Paul Moore

Post by Richard Guy Briggs
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
---
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(-)

I think having some additional context here would be helpful for
everyone, so I agree with this on principle. However, I think we need
to get clarification from Steve that his parser is able to handle
these richer "op" values.

Op fields are not searchable. So, they normally don't matter. But in general,
once they are defined, they should not change. For the record, you can
generally insert non-searchable fields anywhere and it doesn't matter. Only
the searchable fields like loginuid, uid, pid, exe, etc matter to the parser.

-Steve

[Richard Guy Briggs]

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

Move the EOE record creation out of audit_log_exit() and into its
callers __audit_free() and __audit_syscall_exit() so that
audit_kill_trees() can be called prior to the EOE record creation and
any purged trees CONFIG_CHANGE records included in the syscall record
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 <***@redhat.com>
---
kernel/auditsc.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 2590c9e..d56aead 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1460,10 +1460,6 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts

audit_log_proctitle(tsk, context);

- /* Send end of event record to help user space know we are finished */
- ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
- if (ab)
- audit_log_end(ab);
if (call_panic)
audit_panic("error converting sid to string");
}
@@ -1491,6 +1487,14 @@ void __audit_free(struct task_struct *tsk)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
audit_kill_trees(context);
+ if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
+ struct audit_buffer *ab;
+
+ /* Send end of event record to help user space know we are finished */
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
+ if (ab)
+ audit_log_end(ab);
+ }

audit_free_context(context);
}
@@ -1572,13 +1576,19 @@ void __audit_syscall_exit(int success, long return_code)

if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, current);
+ if (!list_empty(&context->killed_trees))
+ audit_kill_trees(context);
+ if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
+ struct audit_buffer *ab;

+ /* Send end of event record to help user space know we are finished */
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
+ if (ab)
+ audit_log_end(ab);
+ }
context->in_syscall = 0;
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;

- if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
-
audit_free_names(context);
unroll_tree_refs(context, NULL, 0);
audit_free_aux(context);

[Paul Moore]

Post by Richard Guy Briggs
The EOE record was being issued prior to the pruning of the killed_tree
list.
Move the EOE record creation out of audit_log_exit() and into its
callers __audit_free() and __audit_syscall_exit() so that
audit_kill_trees() can be called prior to the EOE record creation and
any purged trees CONFIG_CHANGE records included in the syscall record
event.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
---
kernel/auditsc.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)

See my comments in 4/6. Assuming we are able to shuffle the ordering
of audit_log_exit() and audit_kill_trees() this patch would no longer
be needed, yes?

Post by Richard Guy Briggs
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 2590c9e..d56aead 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1460,10 +1460,6 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
audit_log_proctitle(tsk, context);
- /* Send end of event record to help user space know we are finished */
- ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
- if (ab)
- audit_log_end(ab);
if (call_panic)
audit_panic("error converting sid to string");
}
@@ -1491,6 +1487,14 @@ void __audit_free(struct task_struct *tsk)
audit_log_exit(context, tsk);
if (!list_empty(&context->killed_trees))
audit_kill_trees(context);
+ if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
+ struct audit_buffer *ab;
+
+ /* Send end of event record to help user space know we are finished */
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
+ if (ab)
+ audit_log_end(ab);
+ }
audit_free_context(context);
}
@@ -1572,13 +1576,19 @@ void __audit_syscall_exit(int success, long return_code)
if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit(context, current);
+ if (!list_empty(&context->killed_trees))
+ audit_kill_trees(context);
+ if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
+ struct audit_buffer *ab;
+ /* Send end of event record to help user space know we are finished */
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
+ if (ab)
+ audit_log_end(ab);
+ }
context->in_syscall = 0;
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
- if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
-
audit_free_names(context);
unroll_tree_refs(context, NULL, 0);
audit_free_aux(context);
--
1.8.3.1