apparmor: fix race in unix socket mediation when peer_path is used

The holding a reference to the peer_sk is not enough to ensure access
to the peer sk path. Accessing the path outside of the state lock
allows for a race with unix_release_sock(). Fix this by taking the
state lock and getting a reference to the path under lock.

Ideally for connected sockets we would cache this information so we
don't have to take the lock here. But for now just fix the race.

Fixes: bc6e5f6933b8e ("apparmor: Remove use of the double lock")
Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index 09ce3f6da20ce75187013b268c00bee78ca76fde..23753cb2096e53e2ea07f18578f32da2ac3edf8d 100644 (file)
--- a/security/apparmor/af_unix.c
+++ b/security/apparmor/af_unix.c
@@ -748,41 +748,47 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct aa_label *label,
        if (!peer_sk)
                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)) {
-               last_error(error,
-                          unix_fs_perm(op, request, subj_cred, label,
-                                       is_unix_fs(peer_sk) ? &peer_path : NULL));
-       } else if (!is_sk_fs) {
-               struct aa_sk_ctx *pctx = aa_sock(peer_sk);
-
-               rcu_read_lock();
-               plabel = aa_get_newest_label(pctx->label);
-               rcu_read_unlock();
-               /* no fs check of aa_unix_peer_perm because conditions above
-                * ensure they will never be done
-                */
-               last_error(error,
-                       xcheck(unix_peer_perm(subj_cred, label, op,
+       if (!is_sk_fs) {
+               bool is_peer_fs = is_unix_fs(peer_sk);
+
+               peer_addr = aa_sunaddr(unix_sk(peer_sk), &peer_addrlen);
+               if (is_peer_fs) {
+                       struct path peer_path;
+
+                       unix_state_lock(peer_sk);
+                       peer_path = unix_sk(peer_sk)->path;
+                       if (peer_path.dentry)
+                               path_get(&peer_path);
+                       unix_state_unlock(peer_sk);
+
+                       last_error(error,
+                                  unix_fs_perm(op, request, subj_cred, label,
+                                               &peer_path));
+                       if (peer_path.dentry)
+                               path_put(&peer_path);
+               } else {
+                       struct aa_sk_ctx *pctx = aa_sock(peer_sk);
+
+                       rcu_read_lock();
+                       plabel = aa_get_newest_label(pctx->label);
+                       rcu_read_unlock();
+                       /* no fs check of aa_unix_peer_perm because conditions
+                        * above ensure they will never be done
+                        */
+                       last_error(error,
+                               xcheck(unix_peer_perm(subj_cred, label, op,
                                              MAY_READ | MAY_WRITE, sock->sk,
                                              is_sk_fs ? &path : NULL,
                                              peer_addr, peer_addrlen,
-                                             is_unix_fs(peer_sk) ?
-                                                       &peer_path : NULL,
-                                             plabel),
-                              unix_peer_perm(file->f_cred, plabel, op,
+                                             NULL, plabel),
+                                      unix_peer_perm(file->f_cred, plabel, op,
                                              MAY_READ | MAY_WRITE, peer_sk,
-                                             is_unix_fs(peer_sk) ?
-                                                       &peer_path : NULL,
-                                             addr, addrlen,
+                                             NULL, addr, addrlen,
                                              is_sk_fs ? &path : NULL,
                                              label)));
-               if (!error && !__aa_subj_label_is_cached(plabel, label))
-                       update_peer_ctx(peer_sk, pctx, label);
+                       if (!error && !__aa_subj_label_is_cached(plabel, label))
+                               update_peer_ctx(peer_sk, pctx, label);
+               }
        }
        sock_put(peer_sk);