From mboxrd@z Thu Jan  1 00:00:00 1970
From: Steve Grubb <sgrubb@redhat.com>
Subject: Re: [PATCH v2] audit: Allow auditd to set pid to 0 to end auditing
Date: Wed, 18 Oct 2017 18:49:13 -0400
Message-ID: <1783879.P869yhf1d1@x2>
References: <10263350.LhAQzorLlb@x2>
	<CAHC9VhS2bpckOA3Dm+w8gHXzmP8KH-hXK6+5Qc=_p2d8XEu3ng@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <linux-audit-bounces@redhat.com>
In-Reply-To: <CAHC9VhS2bpckOA3Dm+w8gHXzmP8KH-hXK6+5Qc=_p2d8XEu3ng@mail.gmail.com>
List-Unsubscribe: <https://www.redhat.com/mailman/options/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/linux-audit>
List-Post: <mailto:linux-audit@redhat.com>
List-Help: <mailto:linux-audit-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=subscribe>
Sender: linux-audit-bounces@redhat.com
Errors-To: linux-audit-bounces@redhat.com
To: Paul Moore <paul@paul-moore.com>
Cc: rgb@redhat.com, Linux Audit <linux-audit@redhat.com>
List-Id: linux-audit@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