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 <[email protected]> --- 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
