Discussion:
[PATCH v2] audit: Allow auditd to set pid to 0 to end auditing
(too old to reply)
Steve Grubb
2017-10-17 22:29:22 UTC
Permalink
Raw Message
The API to end auditing has historically been for auditd to set the
pid to 0. This patch restores that functionality.

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

Reviewed-by: Richard Guy Briggs <***@redhat.com>
Signed-off-by: Steve Grubb <***@redhat.com>
---
kernel/audit.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 6dd556931739..f6d5fc1d8eb4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1197,25 +1197,28 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
pid_t auditd_pid;
struct pid *req_pid = task_tgid(current);

- /* sanity check - PID values must match */
- if (new_pid != pid_vnr(req_pid))
+ /* Sanity check - PID values must match. Setting
+ * pid to 0 is how auditd ends auditing. */
+ if (new_pid && (new_pid != pid_vnr(req_pid)))
return -EINVAL;

/* test the auditd connection */
audit_replace(req_pid);

auditd_pid = auditd_pid_vnr();
- /* only the current auditd can unregister itself */
- if ((!new_pid) && (new_pid != auditd_pid)) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EACCES;
- }
- /* replacing a healthy auditd is not allowed */
- if (auditd_pid && new_pid) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EEXIST;
+ if (auditd_pid) {
+ /* replacing a healthy auditd is not allowed */
+ if (new_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EEXIST;
+ }
+ /* only current auditd can unregister itself */
+ if (pid_vnr(req_pid) != auditd_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EACCES;
+ }
}

if (new_pid) {
--
2.13.6
Paul Moore
2017-10-18 22:31:47 UTC
Permalink
Raw Message
Post by Steve Grubb
The API to end auditing has historically been for auditd to set the
pid to 0. This patch restores that functionality.
See: https://github.com/linux-audit/audit-kernel/issues/69
A bit of kernel patch etiquette: if you make significant changes to a
patch that has been previously tagged as "Acked-by", "Tested-by", or
"Reviewed-by" it is considered polite to remove that tag in the new
patch as the previous acks/tags/etc. really are no longer valid (at
least that is my take on it). If Richard wants to re-review this new
patch we can re-add the tag (I add the tags when I merge the patch).
Post by Steve Grubb
---
kernel/audit.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 6dd556931739..f6d5fc1d8eb4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1197,25 +1197,28 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
pid_t auditd_pid;
struct pid *req_pid = task_tgid(current);
- /* sanity check - PID values must match */
- if (new_pid != pid_vnr(req_pid))
+ /* Sanity check - PID values must match. Setting
+ * pid to 0 is how auditd ends auditing. */
+ if (new_pid && (new_pid != pid_vnr(req_pid)))
return -EINVAL;
/* test the auditd connection */
audit_replace(req_pid);
auditd_pid = auditd_pid_vnr();
- /* only the current auditd can unregister itself */
- if ((!new_pid) && (new_pid != auditd_pid)) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EACCES;
- }
- /* replacing a healthy auditd is not allowed */
- if (auditd_pid && new_pid) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EEXIST;
+ if (auditd_pid) {
+ /* replacing a healthy auditd is not allowed */
+ if (new_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EEXIST;
+ }
+ /* only current auditd can unregister itself */
+ if (pid_vnr(req_pid) != auditd_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EACCES;
+ }
I realize that you reordered the checks to simplify the conditionals,
but you did reorder the checks ... I'm thinking out loud right now
trying to figure out if that really matters ... probably not,
especially since the checks were broken anyway ... and you need
CAP_AUDIT_CONTROL to even get this far ... we're probably okay.

FWIW, this is something else that is usually best noted in the patch
description. When in doubt, be very verbose in the patch description;
I've never rejected a patch because the description was too lengthy,
but I have rejected patches because there wasn't enough explanation.

Anyway, this looks okay to me, I'll give it another day to see if
Richard wants to re-review it, otherwise I'll strip his reviewed-by
tag and merge it.
Post by Steve Grubb
}
if (new_pid) {
--
paul moore
www.paul-moore.com
Steve Grubb
2017-10-18 22:49:13 UTC
Permalink
Raw Message
Post by Paul Moore
Post by Steve Grubb
auditd_pid = auditd_pid_vnr();
- /* only the current auditd can unregister itself */
- if ((!new_pid) && (new_pid != auditd_pid)) {
- audit_log_config_change("audit_pid",
new_pid, -
auditd_pid, 0); - return -EACCES;
- }
- /* replacing a healthy auditd is not allowed */
- if (auditd_pid && new_pid) {
- audit_log_config_change("audit_pid",
new_pid, -
auditd_pid, 0); - return -EEXIST;
+ if (auditd_pid) {
+ /* replacing a healthy auditd is not
allowed */ + if (new_pid) {
+
audit_log_config_change("audit_pid", +
new_pid, auditd_pid, 0); +
return -EEXIST;
+ }
+ /* only current auditd can unregister
itself */ + if (pid_vnr(req_pid) !=
auditd_pid) { +
audit_log_config_change("audit_pid", +
new_pid, auditd_pid, 0); +
return -EACCES;
+ }
I realize that you reordered the checks to simplify the conditionals,
but you did reorder the checks ... I'm thinking out loud right now
trying to figure out if that really matters ... probably not,
especially since the checks were broken anyway ... and you need
CAP_AUDIT_CONTROL to even get this far ... we're probably okay.
Yes when refactoring as you suggested I realized that we can also remove some
checks for new_pid == 0 because if its not, it takes the first "if" which
returns. Therefore new_pid is guaranteed to be 0 and no check for that is
needed. :-)

-Steve
Richard Guy Briggs
2017-10-19 15:39:52 UTC
Permalink
Raw Message
Post by Paul Moore
Post by Steve Grubb
The API to end auditing has historically been for auditd to set the
pid to 0. This patch restores that functionality.
See: https://github.com/linux-audit/audit-kernel/issues/69
A bit of kernel patch etiquette: if you make significant changes to a
patch that has been previously tagged as "Acked-by", "Tested-by", or
"Reviewed-by" it is considered polite to remove that tag in the new
patch as the previous acks/tags/etc. really are no longer valid (at
least that is my take on it). If Richard wants to re-review this new
patch we can re-add the tag (I add the tags when I merge the patch).
Post by Steve Grubb
---
kernel/audit.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 6dd556931739..f6d5fc1d8eb4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1197,25 +1197,28 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
pid_t auditd_pid;
struct pid *req_pid = task_tgid(current);
- /* sanity check - PID values must match */
- if (new_pid != pid_vnr(req_pid))
+ /* Sanity check - PID values must match. Setting
+ * pid to 0 is how auditd ends auditing. */
+ if (new_pid && (new_pid != pid_vnr(req_pid)))
return -EINVAL;
/* test the auditd connection */
audit_replace(req_pid);
auditd_pid = auditd_pid_vnr();
- /* only the current auditd can unregister itself */
- if ((!new_pid) && (new_pid != auditd_pid)) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EACCES;
- }
- /* replacing a healthy auditd is not allowed */
- if (auditd_pid && new_pid) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EEXIST;
+ if (auditd_pid) {
+ /* replacing a healthy auditd is not allowed */
+ if (new_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EEXIST;
+ }
+ /* only current auditd can unregister itself */
+ if (pid_vnr(req_pid) != auditd_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EACCES;
+ }
I realize that you reordered the checks to simplify the conditionals,
but you did reorder the checks ... I'm thinking out loud right now
trying to figure out if that really matters ... probably not,
especially since the checks were broken anyway ... and you need
CAP_AUDIT_CONTROL to even get this far ... we're probably okay.
FWIW, this is something else that is usually best noted in the patch
description. When in doubt, be very verbose in the patch description;
I've never rejected a patch because the description was too lengthy,
but I have rejected patches because there wasn't enough explanation.
Anyway, this looks okay to me, I'll give it another day to see if
Richard wants to re-review it, otherwise I'll strip his reviewed-by
tag and merge it.
Reviewing...
Post by Paul Moore
Post by Steve Grubb
}
if (new_pid) {
--
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
Richard Guy Briggs
2017-10-19 17:45:40 UTC
Permalink
Raw Message
Post by Richard Guy Briggs
Post by Paul Moore
Post by Steve Grubb
The API to end auditing has historically been for auditd to set the
pid to 0. This patch restores that functionality.
See: https://github.com/linux-audit/audit-kernel/issues/69
A bit of kernel patch etiquette: if you make significant changes to a
patch that has been previously tagged as "Acked-by", "Tested-by", or
"Reviewed-by" it is considered polite to remove that tag in the new
patch as the previous acks/tags/etc. really are no longer valid (at
least that is my take on it). If Richard wants to re-review this new
patch we can re-add the tag (I add the tags when I merge the patch).
Post by Steve Grubb
---
kernel/audit.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 6dd556931739..f6d5fc1d8eb4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1197,25 +1197,28 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
pid_t auditd_pid;
struct pid *req_pid = task_tgid(current);
- /* sanity check - PID values must match */
- if (new_pid != pid_vnr(req_pid))
+ /* Sanity check - PID values must match. Setting
+ * pid to 0 is how auditd ends auditing. */
+ if (new_pid && (new_pid != pid_vnr(req_pid)))
return -EINVAL;
/* test the auditd connection */
audit_replace(req_pid);
auditd_pid = auditd_pid_vnr();
- /* only the current auditd can unregister itself */
- if ((!new_pid) && (new_pid != auditd_pid)) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EACCES;
- }
- /* replacing a healthy auditd is not allowed */
- if (auditd_pid && new_pid) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EEXIST;
+ if (auditd_pid) {
+ /* replacing a healthy auditd is not allowed */
+ if (new_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EEXIST;
+ }
+ /* only current auditd can unregister itself */
+ if (pid_vnr(req_pid) != auditd_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EACCES;
+ }
I realize that you reordered the checks to simplify the conditionals,
but you did reorder the checks ... I'm thinking out loud right now
trying to figure out if that really matters ... probably not,
especially since the checks were broken anyway ... and you need
CAP_AUDIT_CONTROL to even get this far ... we're probably okay.
FWIW, this is something else that is usually best noted in the patch
description. When in doubt, be very verbose in the patch description;
I've never rejected a patch because the description was too lengthy,
but I have rejected patches because there wasn't enough explanation.
Anyway, this looks okay to me, I'll give it another day to see if
Richard wants to re-review it, otherwise I'll strip his reviewed-by
tag and merge it.
Reviewing...
Looks good to me. The re-ordering looks fine. Re-ack.
Post by Richard Guy Briggs
Post by Paul Moore
Post by Steve Grubb
}
if (new_pid) {
--
paul moore
www.paul-moore.com
- RGB
- 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
2017-10-19 19:39:48 UTC
Permalink
Raw Message
Post by Steve Grubb
The API to end auditing has historically been for auditd to set the
pid to 0. This patch restores that functionality.
See: https://github.com/linux-audit/audit-kernel/issues/69
---
kernel/audit.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
As a FYI, I'm not sure how you are sending patches, but however you
are doing it appears to mangling the patch with word wrap. Because
I'm a nice guy, I'm going to go ahead and fix this up (apply by hand),
but in the future you'll need to make that the patches can be applied
straight from your email.
Post by Steve Grubb
diff --git a/kernel/audit.c b/kernel/audit.c
index 6dd556931739..f6d5fc1d8eb4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1197,25 +1197,28 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
pid_t auditd_pid;
struct pid *req_pid = task_tgid(current);
- /* sanity check - PID values must match */
- if (new_pid != pid_vnr(req_pid))
+ /* Sanity check - PID values must match. Setting
+ * pid to 0 is how auditd ends auditing. */
+ if (new_pid && (new_pid != pid_vnr(req_pid)))
return -EINVAL;
/* test the auditd connection */
audit_replace(req_pid);
auditd_pid = auditd_pid_vnr();
- /* only the current auditd can unregister itself */
- if ((!new_pid) && (new_pid != auditd_pid)) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EACCES;
- }
- /* replacing a healthy auditd is not allowed */
- if (auditd_pid && new_pid) {
- audit_log_config_change("audit_pid", new_pid,
- auditd_pid, 0);
- return -EEXIST;
+ if (auditd_pid) {
+ /* replacing a healthy auditd is not allowed */
+ if (new_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EEXIST;
+ }
+ /* only current auditd can unregister itself */
+ if (pid_vnr(req_pid) != auditd_pid) {
+ audit_log_config_change("audit_pid",
+ new_pid, auditd_pid, 0);
+ return -EACCES;
+ }
}
if (new_pid) {
--
2.13.6
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
Loading...