From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Guy Briggs Subject: Re: [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t Date: Mon, 10 Apr 2017 00:30:34 -0400 Message-ID: <20170410043034.GD1572@madcap2.tricolour.ca> References: <149012253566.14936.15800724055046569015.stgit@sifl> <149012274571.14936.3505535245970325863.stgit@sifl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <149012274571.14936.3505535245970325863.stgit@sifl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com On 2017-03-21 14:59, Paul Moore wrote: > From: Paul Moore > > 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 > --- > 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 > #include > #include > +#include > > #include > > @@ -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 -- Richard Guy Briggs 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