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: c05e705812d1 ("apparmor: add fine grained af_unix mediation")
Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index ed4b34b88e3889e51fc4892d9af2735e79746804..53ccf9becdf709da9ed6aba99d0851a4da959c1a 100644 (file)
--- a/security/apparmor/af_unix.c
+++ b/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,19 +434,6 @@ 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)
 {
@@ -456,7 +441,12 @@ int aa_unix_sock_perm(const char *op, u32 request, struct socket *sock)
        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;
@@ -464,7 +454,7 @@ int aa_unix_sock_perm(const char *op, u32 request, struct socket *sock)
 
 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;
 }

diff --git a/security/apparmor/include/af_unix.h b/security/apparmor/include/af_unix.h
index 28390eec3204cd7e79ba1ac8116eb61312ae0941..760d98132392c2f19bc1957072757ee446b7da14 100644 (file)
--- a/security/apparmor/include/af_unix.h
+++ b/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,

diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h
index e27229349abbe1f293f1133254325c622cce9069..365bc67dd15090304400143f9795744a326f1680 100644 (file)
--- a/security/apparmor/include/audit.h
+++ b/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;

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 990211381319a958d85293def45ec7cce3f9e622..0b53ac1c2d70f2edb0e1b269a049a1cf9f83baf6 100644 (file)
--- a/security/apparmor/lsm.c
+++ b/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,

diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index a256a4664826ec7e4262854011fa7311b8101ffb..e6f9e11eaa6a14e59020b0abc5bafce034fef38e 100644 (file)
--- a/security/apparmor/net.c
+++ b/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) {