Re: [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t

[Richard Guy Briggs <rgb@redhat.com>]

On 2017-03-21 14:59, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
> 
> This is arguably the right thing to do, and will make it easier when
> we start supporting multiple audit daemons in different namespaces.

I had tried this several years ago inspired by Eric Biederman's work for
the same reasons:
	https://www.redhat.com/archives/linux-audit/2014-February/msg00116.html

A lot has changed since then...  A couple of comments in-line...

> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  kernel/audit.c |   84 ++++++++++++++++++++++++++++++++++++++------------------
>  kernel/audit.h |    2 +
>  2 files changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6cbf47a372e8..b718bf3a73f8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -58,6 +58,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/mutex.h>
>  #include <linux/gfp.h>
> +#include <linux/pid.h>
>  
>  #include <linux/audit.h>
>  
> @@ -117,7 +118,7 @@ struct audit_net {
>   * or the included spinlock for writing.
>   */
>  static struct auditd_connection {
> -	int pid;
> +	struct pid *pid;
>  	u32 portid;
>  	struct net *net;
>  	spinlock_t lock;
> @@ -221,18 +222,41 @@ struct audit_reply {
>   * Description:
>   * Return 1 if the task is a registered audit daemon, 0 otherwise.
>   */
> -int auditd_test_task(const struct task_struct *task)
> +int auditd_test_task(struct task_struct *task)

Does the compiler complain if this is left as const?

>  {
>  	int rc;
>  
>  	rcu_read_lock();
> -	rc = (auditd_conn.pid && task->tgid == auditd_conn.pid ? 1 : 0);
> +	rc = (auditd_conn.pid && auditd_conn.pid == task_tgid(task) ? 1 : 0);
>  	rcu_read_unlock();
>  
>  	return rc;
>  }
>  
>  /**
> + * auditd_pid_vnr - Return the auditd PID relative to the namespace
> + * @auditd: the auditd connection
> + *
> + * Description:
> + * Returns the PID in relation to the namespace, 0 on failure.  This function
> + * takes the RCU read lock internally, but if the caller needs to protect the
> + * auditd_connection pointer it should take the RCU read lock as well.
> + */
> +static pid_t auditd_pid_vnr(const struct auditd_connection *auditd)
> +{
> +	pid_t pid;
> +
> +	rcu_read_lock();
> +	if (!auditd || !auditd->pid)
> +		pid = 0;
> +	else
> +		pid = pid_vnr(auditd->pid);
> +	rcu_read_unlock();
> +
> +	return pid;
> +}
> +
> +/**
>   * audit_get_sk - Return the audit socket for the given network namespace
>   * @net: the destination network namespace
>   *
> @@ -429,12 +453,17 @@ static int audit_set_failure(u32 state)
>   * This function will obtain and drop network namespace references as
>   * necessary.
>   */
> -static void auditd_set(int pid, u32 portid, struct net *net)
> +static void auditd_set(struct pid *pid, u32 portid, struct net *net)
>  {
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&auditd_conn.lock, flags);
> -	auditd_conn.pid = pid;
> +	if (auditd_conn.pid)
> +		put_pid(auditd_conn.pid);
> +	if (pid)
> +		auditd_conn.pid = get_pid(pid);
> +	else
> +		auditd_conn.pid = NULL;
>  	auditd_conn.portid = portid;
>  	if (auditd_conn.net)
>  		put_net(auditd_conn.net);
> @@ -1062,11 +1091,13 @@ static int audit_set_feature(struct sk_buff *skb)
>  	return 0;
>  }
>  
> -static int audit_replace(pid_t pid)
> +static int audit_replace(struct pid *pid)
>  {
> +	pid_t pvnr;
>  	struct sk_buff *skb;
>  
> -	skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
> +	pvnr = pid_vnr(pid);
> +	skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pvnr, sizeof(pvnr));
>  	if (!skb)
>  		return -ENOMEM;
>  	return auditd_send_unicast_skb(skb);
> @@ -1096,9 +1127,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  		memset(&s, 0, sizeof(s));
>  		s.enabled		= audit_enabled;
>  		s.failure		= audit_failure;
> -		rcu_read_lock();
> -		s.pid			= auditd_conn.pid;
> -		rcu_read_unlock();
> +		/* NOTE: use pid_vnr() so the PID is relative to the current
> +		 *       namespace */
> +		s.pid			= auditd_pid_vnr(&auditd_conn);

I thought this had been fixed earlier (maybe it was in an abandonned
patch) or more likely due to the current state of CAP_AUDIT_CONTROL and
initial PID namespace requirements it was deemed unnecessary.

>  		s.rate_limit		= audit_rate_limit;
>  		s.backlog_limit		= audit_backlog_limit;
>  		s.lost			= atomic_read(&audit_lost);
> @@ -1124,36 +1155,36 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  				return err;
>  		}
>  		if (s.mask & AUDIT_STATUS_PID) {
> -			/* NOTE: we are using task_tgid_vnr() below because
> -			 *       the s.pid value is relative to the namespace
> -			 *       of the caller; at present this doesn't matter
> -			 *       much since you can really only run auditd
> -			 *       from the initial pid namespace, but something
> -			 *       to keep in mind if this changes */
> -			int new_pid = s.pid;
> +			/* NOTE: we are using the vnr PID functions below
> +			 *       because the s.pid value is relative to the
> +			 *       namespace of the caller; at present this
> +			 *       doesn't matter much since you can really only
> +			 *       run auditd from the initial pid namespace, but
> +			 *       something to keep in mind if this changes */
> +			pid_t new_pid = s.pid;
>  			pid_t auditd_pid;
> -			pid_t requesting_pid = task_tgid_vnr(current);
> +			struct pid *req_pid = task_tgid(current);
> +
> +			/* sanity check - PID values must match */
> +			if (new_pid != pid_vnr(req_pid))
> +				return -EINVAL;
>  
>  			/* test the auditd connection */
> -			audit_replace(requesting_pid);
> +			audit_replace(req_pid);
>  
> -			rcu_read_lock();
> -			auditd_pid = auditd_conn.pid;
> +			auditd_pid = auditd_pid_vnr(&auditd_conn);
>  			/* only the current auditd can unregister itself */
> -			if ((!new_pid) && (requesting_pid != auditd_pid)) {
> -				rcu_read_unlock();
> +			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) {
> -				rcu_read_unlock();
>  				audit_log_config_change("audit_pid", new_pid,
>  							auditd_pid, 0);
>  				return -EEXIST;
>  			}
> -			rcu_read_unlock();
>  
>  			if (audit_enabled != AUDIT_OFF)
>  				audit_log_config_change("audit_pid", new_pid,
> @@ -1161,8 +1192,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  
>  			if (new_pid) {
>  				/* register a new auditd connection */
> -				auditd_set(new_pid,
> -					   NETLINK_CB(skb).portid,
> +				auditd_set(req_pid, NETLINK_CB(skb).portid,
>  					   sock_net(NETLINK_CB(skb).sk));
>  				/* try to process any backlog */
>  				wake_up_interruptible(&kauditd_wait);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c21b74dd7ff2..d9b9af769128 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -218,7 +218,7 @@ extern void audit_log_name(struct audit_context *context,
>  			   struct audit_names *n, const struct path *path,
>  			   int record_num, int *call_panic);
>  
> -extern int auditd_test_task(const struct task_struct *task);
> +extern int auditd_test_task(struct task_struct *task);
>  
>  #define AUDIT_INODE_BUCKETS	32
>  extern struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635