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

On 2017-10-18 22:31, Paul Moore wrote: > 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. Reviewing...

> > } > > > > if (new_pid) { > > -- > paul moore > www.paul-moore.com - RGB