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

[Richard Guy Briggs]

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 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...

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 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