* [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing @ 2017-10-17 22:29 Steve Grubb 2017-10-18 22:31 ` Paul Moore 2017-10-19 19:39 ` Paul Moore 0 siblings, 2 replies; 6+ messages in thread From: Steve Grubb @ 2017-10-17 22:29 UTC (permalink / raw) To: Linux Audit 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 <rgb@redhat.com> Signed-off-by: Steve Grubb <sgrubb@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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing 2017-10-17 22:29 [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing Steve Grubb @ 2017-10-18 22:31 ` Paul Moore 2017-10-18 22:49 ` Steve Grubb 2017-10-19 15:39 ` Richard Guy Briggs 2017-10-19 19:39 ` Paul Moore 1 sibling, 2 replies; 6+ messages in thread From: Paul Moore @ 2017-10-18 22:31 UTC (permalink / raw) To: Steve Grubb; +Cc: rgb, Linux Audit On Tue, Oct 17, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote: > 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 <rgb@redhat.com> 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). > Signed-off-by: Steve Grubb <sgrubb@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; > + } 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. > } > > if (new_pid) { -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing 2017-10-18 22:31 ` Paul Moore @ 2017-10-18 22:49 ` Steve Grubb 2017-10-19 15:39 ` Richard Guy Briggs 1 sibling, 0 replies; 6+ messages in thread From: Steve Grubb @ 2017-10-18 22:49 UTC (permalink / raw) To: Paul Moore; +Cc: rgb, Linux Audit On Wednesday, October 18, 2017 6:31:47 PM EDT Paul Moore wrote: > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing 2017-10-18 22:31 ` Paul Moore 2017-10-18 22:49 ` Steve Grubb @ 2017-10-19 15:39 ` Richard Guy Briggs 2017-10-19 17:45 ` Richard Guy Briggs 1 sibling, 1 reply; 6+ messages in thread From: Richard Guy Briggs @ 2017-10-19 15:39 UTC (permalink / raw) To: Paul Moore; +Cc: Linux Audit On 2017-10-18 22:31, Paul Moore wrote: > On Tue, Oct 17, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote: > > 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 <rgb@redhat.com> > > 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). > > > Signed-off-by: Steve Grubb <sgrubb@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; > > + } > > 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... > > } > > > > if (new_pid) { > > -- > paul moore > www.paul-moore.com - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing 2017-10-19 15:39 ` Richard Guy Briggs @ 2017-10-19 17:45 ` Richard Guy Briggs 0 siblings, 0 replies; 6+ messages in thread From: Richard Guy Briggs @ 2017-10-19 17:45 UTC (permalink / raw) To: Paul Moore; +Cc: Linux Audit On 2017-10-19 15:39, Richard Guy Briggs wrote: > On 2017-10-18 22:31, Paul Moore wrote: > > On Tue, Oct 17, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote: > > > 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 <rgb@redhat.com> > > > > 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). > > > > > Signed-off-by: Steve Grubb <sgrubb@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; > > > + } > > > > 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. > > > } > > > > > > if (new_pid) { > > > > -- > > paul moore > > www.paul-moore.com > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing 2017-10-17 22:29 [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing Steve Grubb 2017-10-18 22:31 ` Paul Moore @ 2017-10-19 19:39 ` Paul Moore 1 sibling, 0 replies; 6+ messages in thread From: Paul Moore @ 2017-10-19 19:39 UTC (permalink / raw) To: Steve Grubb; +Cc: Linux Audit On Tue, Oct 17, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote: > 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 <rgb@redhat.com> > Signed-off-by: Steve Grubb <sgrubb@redhat.com> > --- > 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. > 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 > Linux-audit@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-19 19:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-17 22:29 [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing Steve Grubb 2017-10-18 22:31 ` Paul Moore 2017-10-18 22:49 ` Steve Grubb 2017-10-19 15:39 ` Richard Guy Briggs 2017-10-19 17:45 ` Richard Guy Briggs 2017-10-19 19:39 ` Paul Moore
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.