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

[Richard Guy Briggs]

On 2017-10-18 22:31, Paul Moore wrote:
> On Tue, Oct 17, 2017 at 6:29 PM, Steve Grubb <sgrubb at 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 at 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 at 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 at 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