apparmor: Remove use of the double lock

The use of the double lock is not necessary and problematic. Instead
pull the bits that need locks into their own sections and grab the
needed references.

Fixes: c05e705 ("apparmor: add fine grained af_unix mediation")
Signed-off-by: John Johansen <john.johansen@canonical.com>

Author: jrjohansen

security/apparmor/af_unix.c

@@ -30,11 +30,10 @@ static inline struct sock *aa_unix_sk(struct unix_sock *u)
 }
 
 static int unix_fs_perm(const char *op, u32 mask, const struct cred *subj_cred,
-			struct aa_label *label, struct unix_sock *u)
+			struct aa_label *label, struct path *path)
 {
 	AA_BUG(!label);
-	AA_BUG(!u);
-	AA_BUG(!is_unix_fs(aa_unix_sk(u)));
+	AA_BUG(!path);
 
 	if (unconfined(label) || !label_mediates(label, AA_CLASS_FILE))
 		return 0;
@@ -43,13 +42,13 @@ static int unix_fs_perm(const char *op, u32 mask, const struct cred *subj_cred,
 	/* if !u->path.dentry socket is being shutdown - implicit delegation
 	 * until obj delegation is supported
 	 */
-	if (u->path.dentry) {
+	if (path->dentry) {
 		/* the sunpath may not be valid for this ns so use the path */
-		struct path_cond cond = { u->path.dentry->d_inode->i_uid,
-					  u->path.dentry->d_inode->i_mode
+		struct path_cond cond = { path->dentry->d_inode->i_uid,
+					  path->dentry->d_inode->i_mode
 		};
 
-		return aa_path_perm(op, subj_cred, label, &u->path,
+		return aa_path_perm(op, subj_cred, label, path,
 				    PATH_SOCK_COND, mask, &cond);
 	} /* else implicitly delegated */
 
@@ -102,18 +101,27 @@ static aa_state_t match_to_local(struct aa_policydb *policy,
 	return state;
 }
 
+struct sockaddr_un *aa_sunaddr(const struct unix_sock *u, int *addrlen)
+{
+	struct unix_address *addr;
+
+	/* memory barrier is sufficient see note in net/unix/af_unix.c */
+	addr = smp_load_acquire(&u->addr);
+	if (addr) {
+		*addrlen = addr->len;
+		return addr->name;
+	}
+	*addrlen = 0;
+	return NULL;
+}
+
 static aa_state_t match_to_sk(struct aa_policydb *policy,
 			      aa_state_t state, u32 request,
 			      struct unix_sock *u, struct aa_perms **p,
 			      const char **info)
 {
-	struct sockaddr_un *addr = NULL;
-	int addrlen = 0;
-
-	if (u->addr) {
-		addr = u->addr->name;
-		addrlen = u->addr->len;
-	}
+	int addrlen;
+	struct sockaddr_un *addr = aa_sunaddr(u, &addrlen);
 
 	return match_to_local(policy, state, request, u->sk.sk_type,
 			      u->sk.sk_protocol, addr, addrlen, p, info);
@@ -363,7 +371,8 @@ static int profile_opt_perm(struct aa_profile *profile, u32 request,
 
 /* null peer_label is allowed, in which case the peer_sk label is used */
 static int profile_peer_perm(struct aa_profile *profile, u32 request,
-			     struct sock *sk, struct sock *peer_sk,
+			     struct sock *sk, struct sockaddr_un *peer_addr,
+			     int peer_addrlen,
 			     struct aa_label *peer_label,
 			     struct apparmor_audit_data *ad)
 {
@@ -375,26 +384,16 @@ static int profile_peer_perm(struct aa_profile *profile, u32 request,
 	AA_BUG(!profile);
 	AA_BUG(profile_unconfined(profile));
 	AA_BUG(!sk);
-	AA_BUG(!peer_sk);
+	AA_BUG(!peer_label);
 	AA_BUG(!ad);
-	AA_BUG(is_unix_fs(peer_sk)); /* currently always calls unix_fs_perm */
 
 	state = RULE_MEDIATES_v9NET(rules);
 	if (state) {
-		struct aa_sk_ctx *peer_ctx = aa_sock(peer_sk);
 		struct aa_profile *peerp;
-		struct sockaddr_un *addr = NULL;
-		int len = 0;
 
-		if (unix_sk(peer_sk)->addr) {
-			addr = unix_sk(peer_sk)->addr->name;
-			len = unix_sk(peer_sk)->addr->len;
-		}
 		state = match_to_peer(rules->policy, state, request,
 				      unix_sk(sk),
-				      addr, len, &p, &ad->info);
-		if (!peer_label)
-			peer_label = peer_ctx->label;
+				      peer_addr, peer_addrlen, &p, &ad->info);
 
 		return fn_for_each_in_ns(peer_label, peerp,
 				match_label(profile, rules, state, request,
@@ -422,9 +421,8 @@ int aa_unix_create_perm(struct aa_label *label, int family, int type,
 	return 0;
 }
 
-int aa_unix_label_sk_perm(const struct cred *subj_cred,
-			  struct aa_label *label, const char *op, u32 request,
-			  struct sock *sk)
+int aa_unix_label_sk_perm(const struct cred *subj_cred, struct aa_label *label,
+			  const char *op, u32 request, struct sock *sk)
 {
 	if (!unconfined(label)) {
 		struct aa_profile *profile;
@@ -436,35 +434,27 @@ int aa_unix_label_sk_perm(const struct cred *subj_cred,
 	return 0;
 }
 
-static int unix_label_sock_perm(const struct cred *subj_cred,
-				struct aa_label *label, const char *op,
-				u32 request, struct socket *sock)
-{
-	if (unconfined(label))
-		return 0;
-	if (is_unix_fs(sock->sk))
-		return unix_fs_perm(op, request, subj_cred, label,
-				    unix_sk(sock->sk));
-
-	return aa_unix_label_sk_perm(subj_cred, label, op, request, sock->sk);
-}
-
 /* revalidation, get/set attr, shutdown */
 int aa_unix_sock_perm(const char *op, u32 request, struct socket *sock)
 {
 	struct aa_label *label;
 	int error;
 
 	label = begin_current_label_crit_section();
-	error = unix_label_sock_perm(current_cred(), label, op, request, sock);
+	if (is_unix_fs(sock->sk))
+		error = unix_fs_perm(op, request, current_cred(), label,
+				     &unix_sk(sock->sk)->path);
+	else
+		error = aa_unix_label_sk_perm(current_cred(), label, op,
+					      request, sock->sk);
 	end_current_label_crit_section(label);
 
 	return error;
 }
 
 static int valid_addr(struct sockaddr *addr, int addr_len)
 {
-	struct sockaddr_un *sunaddr = (struct sockaddr_un *)addr;
+	struct sockaddr_un *sunaddr = unix_addr(addr);
 
 	/* addr_len == offsetof(struct sockaddr_un, sun_path) is autobind */
 	if (addr_len < offsetof(struct sockaddr_un, sun_path) ||
@@ -586,6 +576,22 @@ int aa_unix_opt_perm(const char *op, u32 request, struct socket *sock,
 	return error;
 }
 
+static int unix_peer_perm(const struct cred *subj_cred,
+			  struct aa_label *label, const char *op, u32 request,
+			  struct sock *sk, struct sockaddr_un *peer_addr,
+			  int peer_addrlen, struct aa_label *peer_label)
+{
+	struct aa_profile *profile;
+	DEFINE_AUDIT_SK(ad, op, subj_cred, sk);
+
+	ad.net.addr = peer_addr;
+	ad.net.addrlen = peer_addrlen;
+
+	return fn_for_each_confined(label, profile,
+			profile_peer_perm(profile, request, sk,
+					  peer_addr, peer_addrlen, peer_label, &ad));
+}
+
 /**
  *
  * Requires: lock held on both @sk and @peer_sk
@@ -602,58 +608,37 @@ int aa_unix_peer_perm(const struct cred *subj_cred,
 	AA_BUG(!label);
 	AA_BUG(!sk);
 	AA_BUG(!peer_sk);
+	AA_BUG(!peer_label);
 
 	if (is_unix_fs(aa_unix_sk(peeru))) {
-		return unix_fs_perm(op, request, subj_cred, label, peeru);
+		return unix_fs_perm(op, request, subj_cred, label,
+				    &peeru->path);
 	} else if (is_unix_fs(aa_unix_sk(u))) {
-		return unix_fs_perm(op, request, subj_cred, label, u);
+		return unix_fs_perm(op, request, subj_cred, label, &u->path);
 	} else if (!unconfined(label)) {
-		struct aa_profile *profile;
-		DEFINE_AUDIT_SK(ad, op, subj_cred, sk);
-
-		ad.net.peer_sk = peer_sk;
+		int plen;
+		struct sockaddr_un *paddr = aa_sunaddr(unix_sk(peer_sk),
+							       &plen);
 
-		return fn_for_each_confined(label, profile,
-				profile_peer_perm(profile, request, sk,
-						  peer_sk, peer_label, &ad));
+		return unix_peer_perm(subj_cred, label, op, request,
+				      sk, paddr, plen, peer_label);
 	}
 
 	return 0;
 }
 
-static void unix_state_double_lock(struct sock *sk1, struct sock *sk2)
-{
-	if (unlikely(sk1 == sk2) || !sk2) {
-		unix_state_lock(sk1);
-		return;
-	}
-	if (sk1 < sk2) {
-		unix_state_lock(sk1);
-		unix_state_lock(sk2);
-	} else {
-		unix_state_lock(sk2);
-		unix_state_lock(sk1);
-	}
-}
-
-static void unix_state_double_unlock(struct sock *sk1, struct sock *sk2)
-{
-	if (unlikely(sk1 == sk2) || !sk2) {
-		unix_state_unlock(sk1);
-		return;
-	}
-	unix_state_unlock(sk1);
-	unix_state_unlock(sk2);
-}
-
-/* TODO: examine replacing double lock with cached addr */
-
+/* This fn is only checked if something has changed in the security
+ * boundaries. Otherwise cached info off file is sufficient
+ */
 int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 		      const char *op, u32 request, struct file *file)
 {
 	struct socket *sock = (struct socket *) file->private_data;
+	struct sockaddr_un *addr, *peer_addr;
+	int addrlen, peer_addrlen;
 	struct sock *peer_sk = NULL;
 	u32 sk_req = request & ~NET_PEER_MASK;
+	struct path path;
 	bool is_sk_fs;
 	int error = 0;
 
@@ -663,40 +648,60 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 	AA_BUG(sock->sk->sk_family != PF_UNIX);
 
 	/* TODO: update sock label with new task label */
+	/* investigate only using lock via unix_peer_get()
+	 * addr only needs the memory barrier, but need to investigate
+	 * path
+	 */
 	unix_state_lock(sock->sk);
 	peer_sk = unix_peer(sock->sk);
 	if (peer_sk)
 		sock_hold(peer_sk);
 
 	is_sk_fs = is_unix_fs(sock->sk);
+	addr = aa_sunaddr(unix_sk(sock->sk), &addrlen);
+	path = unix_sk(sock->sk)->path;
+	unix_state_unlock(sock->sk);
+
 	if (is_sk_fs && peer_sk)
 		sk_req = request;
-	if (sk_req)
-		error = unix_label_sock_perm(subj_cred, label, op, sk_req,
-					     sock);
-	unix_state_unlock(sock->sk);
+	if (sk_req) {
+		if (is_sk_fs)
+			error = unix_fs_perm(op, sk_req, subj_cred, label,
+					     &path);
+		else
+			error = aa_unix_label_sk_perm(subj_cred, label, op,
+						      sk_req, sock->sk);
+	}
 	if (!peer_sk)
-		return error;
+		goto out;
+
+	peer_addr = aa_sunaddr(unix_sk(peer_sk), &peer_addrlen);
 
-	unix_state_double_lock(sock->sk, peer_sk);
+	struct path peer_path;
+
+	peer_path = unix_sk(peer_sk)->path;
 	if (!is_sk_fs && is_unix_fs(peer_sk)) {
 		last_error(error,
 			   unix_fs_perm(op, request, subj_cred, label,
-					unix_sk(peer_sk)));
+					&peer_path));
 	} else if (!is_sk_fs) {
 		struct aa_sk_ctx *pctx = aa_sock(peer_sk);
 
+		/* no fs check of aa_unix_peer_perm because conditions above
+		 * ensure they will never be done
+		 */
 		last_error(error,
-			xcheck(aa_unix_peer_perm(subj_cred, label, op,
-						 MAY_READ | MAY_WRITE,
-						 sock->sk, peer_sk, NULL),
-			       aa_unix_peer_perm(file->f_cred, pctx->label, op,
-						 MAY_READ | MAY_WRITE,
-						 peer_sk, sock->sk, label)));
+			xcheck(unix_peer_perm(subj_cred, label, op,
+					      MAY_READ | MAY_WRITE, sock->sk,
+					      peer_addr, peer_addrlen,
+					      pctx->label),
+			       unix_peer_perm(file->f_cred, pctx->label, op,
+					      MAY_READ | MAY_WRITE, peer_sk,
+					      addr, addrlen, label)));
 	}
-	unix_state_double_unlock(sock->sk, peer_sk);
-
 	sock_put(peer_sk);
 
+out:
+
 	return error;
 }

security/apparmor/include/af_unix.h

@@ -31,6 +31,7 @@
 #define is_unix_connected(S) ((S)->state == SS_CONNECTED)
 
 
+struct sockaddr_un *aa_sunaddr(const struct unix_sock *u, int *addrlen);
 int aa_unix_peer_perm(const struct cred *subj_cred,
 		      struct aa_label *label, const char *op, u32 request,
 		      struct sock *sk, struct sock *peer_sk,

security/apparmor/include/audit.h

@@ -138,7 +138,6 @@ struct apparmor_audit_data {
 				};
 				struct {
 					int type, protocol;
-					struct sock *peer_sk;
 					void *addr;
 					int addrlen;
 				} net;

security/apparmor/lsm.c

@@ -1112,7 +1112,7 @@ static int unix_connect_perm(const struct cred *cred, struct aa_label *label,
 
 	error = aa_unix_peer_perm(cred, label, OP_CONNECT,
 				(AA_MAY_CONNECT | AA_MAY_SEND | AA_MAY_RECEIVE),
-				  sk, peer_sk, NULL);
+				  sk, peer_sk, peer_ctx->label);
 	if (!is_unix_fs(peer_sk)) {
 		last_error(error,
 			   aa_unix_peer_perm(cred,
@@ -1184,7 +1184,7 @@ static int apparmor_unix_may_send(struct socket *sock, struct socket *peer)
 	label = __begin_current_label_crit_section(&needput);
 	error = xcheck(aa_unix_peer_perm(current_cred(),
 					 label, OP_SENDMSG, AA_MAY_SEND,
-					 sock->sk, peer->sk, NULL),
+					 sock->sk, peer->sk, peer_ctx->label),
 		       aa_unix_peer_perm(peer->file ? peer->file->f_cred : NULL,
 					 peer_ctx->label, OP_SENDMSG,
 					 AA_MAY_RECEIVE,

security/apparmor/net.c

@@ -148,9 +148,6 @@ void audit_net_cb(struct audit_buffer *ab, void *va)
 				audit_unix_addr(ab, "peer_addr",
 						unix_addr(ad->net.addr),
 						ad->net.addrlen);
-			else
-				audit_unix_sk_addr(ab, "peer_addr",
-						   ad->net.peer_sk);
 		}
 	}
 	if (ad->peer) {