From: Steve Grubb <sgrubb@redhat.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org, pmoore@redhat.com,
        eparis@redhat.com, v.rathor@gmail.com, ctcard@hotmail.com
Subject: Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd
Date: Wed, 16 Dec 2015 11:23:19 -0500
Message-ID: <2544274.X8ZVJd57ai@x2>
Organization: Red Hat
User-Agent: KMail/4.14.10 (Linux/4.2.3-203.fc22.x86_64; KDE/4.14.14; x86_64; ; )
In-Reply-To: <735d85abf2484b740fd3aa551cdffde67f9bd637.1450268948.git.rgb@redhat.com>
References: <735d85abf2484b740fd3aa551cdffde67f9bd637.1450268948.git.rgb@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4042
Lines: 105

Hello Richard,

Public reply this time.  :-)

On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> Nothing prevents a new auditd starting up and replacing a valid
> audit_pid when an old auditd is still running, effectively starving out
> the old auditd since audit_pid no longer points to the old valid auditd.

I guess the first question is why do we allow something to start up a new 
auditd without killing off the old one? Would that be a simpler fix?


> If no message to auditd has been attempted since auditd died unnaturally
> or got killed, audit_pid will still indicate it is alive.  There isn't
> an easy way to detect if an old auditd is still running on the existing
> audit_pid other than attempting to send a message to see if it fails.
> An -ECONNREFUSED almost certainly means it disappeared and can be
> replaced.  Other errors are not so straightforward and may indicate
> transient problems that will resolve themselves and the old auditd will
> recover.  Yet others will likely need manual intervention for which a
> new auditd will not solve the problem.
> 
> Send a new message type (AUDIT_REPLACE) to the old auditd containing a
> u32 with the PID of the new auditd.  If the audit replace message
> succeeds (or doesn't fail with certainty), fail to register the new
> auditd and return an error (-EEXIST).
> 
> This is expected to make the patch preventing an old auditd orphaning a
> new auditd redundant.
> 
> V2: Rename audit_ping to audit_replace, set seq and portid to 0 in
> the call to audit_make_reply().
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/uapi/linux/audit.h |    1 +
>  kernel/audit.c             |   16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 843540c..cf84991 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -70,6 +70,7 @@
>  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
>  #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
>  #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
> +#define AUDIT_REPLACE		1020	/* Replace auditd if this packet 
unanswerd */

In every case, events in the 1000 block are to configure the kernel or to ask 
the kernel a question. This is user space initiating. Kernel initiating events 
are in the 1300 block of numbers.

-Steve


>  #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly
> uninteresting to kernel */ #define AUDIT_USER_AVC		1107	/* We filter 
this
> differently */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 36989a1..0368be2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -809,6 +809,16 @@ static int audit_set_feature(struct sk_buff *skb)
>  	return 0;
>  }
> 
> +static int audit_replace(pid_t pid)
> +{
> +	struct sk_buff *skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0,
> +					       &pid, sizeof(pid));
> +
> +	if (!skb)
> +		return -ENOMEM;
> +	return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> +}
> +
>  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  {
>  	u32			seq;
> @@ -870,9 +880,13 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) }
>  		if (s.mask & AUDIT_STATUS_PID) {
>  			int new_pid = s.pid;
> +			pid_t requesting_pid = task_tgid_vnr(current);
> 
> -			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> +			if ((!new_pid) && (requesting_pid != audit_pid))
>  				return -EACCES;
> +			if (audit_pid && new_pid &&
> +			    audit_replace(requesting_pid) != -ECONNREFUSED)
> +				return -EEXIST;
>  			if (audit_enabled != AUDIT_OFF)
>  				audit_log_config_change("audit_pid", new_pid, audit_pid, 
1);
>  			audit_pid = new_pid;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/