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

[Paul Moore]

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.

>                         }
>
>                         if (new_pid) {

-- 
paul moore
www.paul-moore.com