Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing

On Tue, Oct 17, 2017 at 6:29 PM, Steve Grubb <sgrubb(a)redhat.com&gt; 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(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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