Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

Steve Grubb
2015-12-16T16:23:24Z

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;