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

[Steve Grubb <sgrubb@redhat.com>]

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