[PATCH] apparmor: hold peer path references in aa_unix_file_perm()

[PATCH] apparmor: hold peer path references in aa_unix_file_perm()

[Zhang Cen]

aa_unix_file_perm() keeps the connected peer alive with sock_hold(peer_sk),
but it then carries unix_sk(peer_sk)->path outside the peer socket state
lock without taking a path reference. That copied peer_path can race with
unix_release_sock(), which clears u->path under unix_state_lock(peer_sk)
and drops the socket-owned path reference with path_put() before the final
sock_put(peer_sk).

Take peer_sk's unix_state_lock() long enough to snapshot peer_path,
cache whether the peer is filesystem-bound, and path_get() a non-NULL
path before dropping the lock. Drop that path reference after the last
AppArmor peer path check. This restores the ownership invariant for
peer_path without changing AF_UNIX shutdown semantics once the peer path
has already been cleared.

The buggy scenario involves two paths, with each column showing the
order within that path:

aa_unix_file_perm() [borrower]:        unix_release_sock() [peer close]:
1. unix_state_lock(sock->sk)           1. unix_state_lock(peer_sk)
2. peer_sk = unix_peer(sock->sk)       2. Save path = u->path
3. sock_hold(peer_sk)                  3. Clear u->path.dentry/mnt
4. unix_state_unlock(sock->sk)         4. unix_state_unlock(peer_sk)
5. peer_path = unix_sk(peer_sk)->path  5. path_put(&path)
6. unix_fs_perm(&peer_path)            6. sock_put(peer_sk)

KASAN reported a slab-use-after-free in unix_fs_perm() at
security/apparmor/af_unix.c:46, with the free side in
unix_release_sock() -> path_put() at net/unix/af_unix.c:730.

Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>

---
 security/apparmor/af_unix.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index fdb4a9f21..7a1562f6f 100644
--- a/security/apparmor/af_unix.c
+++ b/security/apparmor/af_unix.c
@@ -716,7 +716,8 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 	struct sock *peer_sk = NULL;
 	u32 sk_req = request & ~NET_PEER_MASK;
 	struct path path;
-	bool is_sk_fs;
+	struct path peer_path = {};
+	bool is_sk_fs, is_peer_fs = false;
 	int error = 0;
 
 	AA_BUG(!label);
@@ -724,9 +725,8 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 	AA_BUG(!sock->sk);
 	AA_BUG(sock->sk->sk_family != PF_UNIX);
 
-	/* investigate only using lock via unix_peer_get()
-	 * addr only needs the memory barrier, but need to investigate
-	 * path
+	/* addr only needs the memory barrier; hold a peer path reference
+	 * under peer_sk's state lock after sock_hold(peer_sk)
 	 */
 	unix_state_lock(sock->sk);
 	peer_sk = unix_peer(sock->sk);
@@ -749,14 +749,18 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 		goto out;
 
 	peer_addr = aa_sunaddr(unix_sk(peer_sk), &peer_addrlen);
-
-	struct path peer_path;
-
-	peer_path = unix_sk(peer_sk)->path;
-	if (!is_sk_fs && is_unix_fs(peer_sk)) {
+	if (!is_sk_fs) {
+		unix_state_lock(peer_sk);
+		is_peer_fs = is_unix_fs(peer_sk);
+		peer_path = unix_sk(peer_sk)->path;
+		if (peer_path.dentry)
+			path_get(&peer_path);
+		unix_state_unlock(peer_sk);
+	}
+	if (!is_sk_fs && is_peer_fs) {
 		last_error(error,
 			   unix_fs_perm(op, request, subj_cred, label,
-					is_unix_fs(peer_sk) ? &peer_path : NULL));
+					&peer_path));
 	} else if (!is_sk_fs) {
 		struct aa_label *plabel;
 		struct aa_sk_ctx *pctx = aa_sock(peer_sk);
@@ -772,12 +776,12 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 					      MAY_READ | MAY_WRITE, sock->sk,
 					      is_sk_fs ? &path : NULL,
 					      peer_addr, peer_addrlen,
-					      is_unix_fs(peer_sk) ?
+					      is_peer_fs ?
 							&peer_path : NULL,
 					      plabel),
 			       unix_peer_perm(file->f_cred, plabel, op,
 					      MAY_READ | MAY_WRITE, peer_sk,
-					      is_unix_fs(peer_sk) ?
+					      is_peer_fs ?
 							&peer_path : NULL,
 					      addr, addrlen,
 					      is_sk_fs ? &path : NULL,
@@ -785,6 +789,8 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 		if (!error && !__aa_subj_label_is_cached(plabel, label))
 			update_peer_ctx(peer_sk, pctx, label);
 	}
+	if (peer_path.dentry)
+		path_put(&peer_path);
 	sock_put(peer_sk);
 
 out:
@@ -796,4 +802,3 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
 
 	return error;
 }
-
-- 
2.43.0