ANDROID: Add untag hacks to inet_release function

To prevent protential risk of memory leak caused by closing socket with
out untag it from qtaguid module, the qtaguid module now do not hold any
socket file reference count. Instead, it will increase the sk_refcnt of
the sk struct to prevent a reuse of the socket pointer.  And when a socket
is released. It will delete the tag if the socket is previously tagged so
no more resources is held by xt_qtaguid moudle. A flag is added to the untag
process to prevent possible kernel crash caused by fail to delete
corresponding socket_tag_entry list.
Bug: 36374484
Test: compile and run test under system/extra/test/iptables,
      run cts -m CtsNetTestCases -t android.net.cts.SocketRefCntTest

Signed-off-by: Chenbo Feng <fengc@google.com>
Change-Id: Iea7c3bf0c59b9774a5114af905b2405f6bc9ee52

diff --git a/include/linux/netfilter/xt_qtaguid.h b/include/linux/netfilter/xt_qtaguid.h
index ca60fbdec2f3b056bce47d1c0fe743f7d4712db9..1c671552ec37f962039695e6a1f4cee008a10f26 100644 (file)
--- a/include/linux/netfilter/xt_qtaguid.h
+++ b/include/linux/netfilter/xt_qtaguid.h
@@ -10,4 +10,5 @@
 #define XT_QTAGUID_SOCKET XT_OWNER_SOCKET
 #define xt_qtaguid_match_info xt_owner_match_info
 
+int qtaguid_untag(struct socket *sock, bool kernel);
 #endif /* _XT_QTAGUID_MATCH_H */

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 68bf7bdf7fdb71a7e5abd54e8826e1cfb67e52ec..eb5cfc1478b12083d82dbbb3c861b39ac66b2276 100644 (file)
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -89,6 +89,7 @@
 #include <linux/netfilter_ipv4.h>
 #include <linux/random.h>
 #include <linux/slab.h>
+#include <linux/netfilter/xt_qtaguid.h>
 
 #include <asm/uaccess.h>
 
@@ -413,6 +414,9 @@ int inet_release(struct socket *sock)
        if (sk) {
                long timeout;
 
+#ifdef CONFIG_NETFILTER_XT_MATCH_QTAGUID
+               qtaguid_untag(sock, true);
+#endif
                /* Applications forget to leave groups before exiting */
                ip_mc_drop_socket(sk);
 

diff --git a/net/netfilter/xt_qtaguid.c b/net/netfilter/xt_qtaguid.c
index 0f5628a59917e393f0eec7cb564a16dd6e1c0b22..f95aba44e649ea82ef36ed5f309182fb016f7a56 100644 (file)
--- a/net/netfilter/xt_qtaguid.c
+++ b/net/netfilter/xt_qtaguid.c
@@ -321,7 +321,7 @@ static void sock_tag_tree_erase(struct rb_root *st_to_free_tree)
                         st_entry->tag,
                         get_uid_from_tag(st_entry->tag));
                rb_erase(&st_entry->sock_node, st_to_free_tree);
-               sockfd_put(st_entry->socket);
+               sock_put(st_entry->sk);
                kfree(st_entry);
        }
 }
@@ -1929,12 +1929,12 @@ static int qtaguid_ctrl_proc_show(struct seq_file *m, void *v)
 {
        struct sock_tag *sock_tag_entry = v;
        uid_t uid;
-       long f_count;
 
        CT_DEBUG("qtaguid: proc ctrl pid=%u tgid=%u uid=%u\n",
                 current->pid, current->tgid, from_kuid(&init_user_ns, current_fsuid()));
 
        if (sock_tag_entry != SEQ_START_TOKEN) {
+               int sk_ref_count;
                uid = get_uid_from_tag(sock_tag_entry->tag);
                CT_DEBUG("qtaguid: proc_read(): sk=%p tag=0x%llx (uid=%u) "
                         "pid=%u\n",
@@ -1943,13 +1943,13 @@ static int qtaguid_ctrl_proc_show(struct seq_file *m, void *v)
                         uid,
                         sock_tag_entry->pid
                        );
-               f_count = atomic_long_read(
-                       &sock_tag_entry->socket->file->f_count);
+               sk_ref_count = atomic_read(
+                       &sock_tag_entry->sk->sk_refcnt);
                seq_printf(m, "sock=%pK tag=0x%llx (uid=%u) pid=%u "
-                          "f_count=%lu\n",
+                          "f_count=%d\n",
                           sock_tag_entry->sk,
                           sock_tag_entry->tag, uid,
-                          sock_tag_entry->pid, f_count);
+                          sock_tag_entry->pid, sk_ref_count);
        } else {
                seq_printf(m, "events: sockets_tagged=%llu "
                           "sockets_untagged=%llu "
@@ -2245,8 +2245,8 @@ static int ctrl_cmd_tag(const char *input)
                        from_kuid(&init_user_ns, current_fsuid()));
                goto err;
        }
-       CT_DEBUG("qtaguid: ctrl_tag(%s): socket->...->f_count=%ld ->sk=%p\n",
-                input, atomic_long_read(&el_socket->file->f_count),
+       CT_DEBUG("qtaguid: ctrl_tag(%s): socket->...->sk_refcnt=%d ->sk=%p\n",
+                input, atomic_read(&el_socket->sk->sk_refcnt),
                 el_socket->sk);
        if (argc < 3) {
                acct_tag = make_atag_from_value(0);
@@ -2290,16 +2290,9 @@ static int ctrl_cmd_tag(const char *input)
                struct tag_ref *prev_tag_ref_entry;
 
                CT_DEBUG("qtaguid: ctrl_tag(%s): retag for sk=%p "
-                        "st@%p ...->f_count=%ld\n",
+                        "st@%p ...->sk_refcnt=%d\n",
                         input, el_socket->sk, sock_tag_entry,
-                        atomic_long_read(&el_socket->file->f_count));
-               /*
-                * This is a re-tagging, so release the sock_fd that was
-                * locked at the time of the 1st tagging.
-                * There is still the ref from this call's sockfd_lookup() so
-                * it can be done within the spinlock.
-                */
-               sockfd_put(sock_tag_entry->socket);
+                        atomic_read(&el_socket->sk->sk_refcnt));
                prev_tag_ref_entry = lookup_tag_ref(sock_tag_entry->tag,
                                                    &uid_tag_data_entry);
                BUG_ON(IS_ERR_OR_NULL(prev_tag_ref_entry));
@@ -2319,8 +2312,12 @@ static int ctrl_cmd_tag(const char *input)
                        res = -ENOMEM;
                        goto err_tag_unref_put;
                }
+               /*
+                * Hold the sk refcount here to make sure the sk pointer cannot
+                * be freed and reused
+                */
+               sock_hold(el_socket->sk);
                sock_tag_entry->sk = el_socket->sk;
-               sock_tag_entry->socket = el_socket;
                sock_tag_entry->pid = current->tgid;
                sock_tag_entry->tag = combine_atag_with_uid(acct_tag, uid_int);
                spin_lock_bh(&uid_tag_data_tree_lock);
@@ -2347,10 +2344,11 @@ static int ctrl_cmd_tag(const char *input)
                atomic64_inc(&qtu_events.sockets_tagged);
        }
        spin_unlock_bh(&sock_tag_list_lock);
-       /* We keep the ref to the socket (file) until it is untagged */
-       CT_DEBUG("qtaguid: ctrl_tag(%s): done st@%p ...->f_count=%ld\n",
+       /* We keep the ref to the sk until it is untagged */
+       CT_DEBUG("qtaguid: ctrl_tag(%s): done st@%p ...->sk_refcnt=%d\n",
                 input, sock_tag_entry,
-                atomic_long_read(&el_socket->file->f_count));
+                atomic_read(&el_socket->sk->sk_refcnt));
+       sockfd_put(el_socket);
        return 0;
 
 err_tag_unref_put:
@@ -2358,8 +2356,8 @@ err_tag_unref_put:
        tag_ref_entry->num_sock_tags--;
        free_tag_ref_from_utd_entry(tag_ref_entry, uid_tag_data_entry);
 err_put:
-       CT_DEBUG("qtaguid: ctrl_tag(%s): done. ...->f_count=%ld\n",
-                input, atomic_long_read(&el_socket->file->f_count) - 1);
+       CT_DEBUG("qtaguid: ctrl_tag(%s): done. ...->sk_refcnt=%d\n",
+                input, atomic_read(&el_socket->sk->sk_refcnt) - 1);
        /* Release the sock_fd that was grabbed by sockfd_lookup(). */
        sockfd_put(el_socket);
        return res;
@@ -2375,17 +2373,13 @@ static int ctrl_cmd_untag(const char *input)
        int sock_fd = 0;
        struct socket *el_socket;
        int res, argc;
-       struct sock_tag *sock_tag_entry;
-       struct tag_ref *tag_ref_entry;
-       struct uid_tag_data *utd_entry;
-       struct proc_qtu_data *pqd_entry;
 
        argc = sscanf(input, "%c %d", &cmd, &sock_fd);
        CT_DEBUG("qtaguid: ctrl_untag(%s): argc=%d cmd=%c sock_fd=%d\n",
                 input, argc, cmd, sock_fd);
        if (argc < 2) {
                res = -EINVAL;
-               goto err;
+               return res;
        }
        el_socket = sockfd_lookup(sock_fd, &res);  /* This locks the file */
        if (!el_socket) {
@@ -2393,17 +2387,31 @@ static int ctrl_cmd_untag(const char *input)
                        " sock_fd=%d err=%d pid=%u tgid=%u uid=%u\n",
                        input, sock_fd, res, current->pid, current->tgid,
                        from_kuid(&init_user_ns, current_fsuid()));
-               goto err;
+               return res;
        }
        CT_DEBUG("qtaguid: ctrl_untag(%s): socket->...->f_count=%ld ->sk=%p\n",
                 input, atomic_long_read(&el_socket->file->f_count),
                 el_socket->sk);
+       res = qtaguid_untag(el_socket, false);
+       sockfd_put(el_socket);
+       return res;
+}
+
+int qtaguid_untag(struct socket *el_socket, bool kernel)
+{
+       int res;
+       pid_t pid;
+       struct sock_tag *sock_tag_entry;
+       struct tag_ref *tag_ref_entry;
+       struct uid_tag_data *utd_entry;
+       struct proc_qtu_data *pqd_entry;
+
        spin_lock_bh(&sock_tag_list_lock);
        sock_tag_entry = get_sock_stat_nl(el_socket->sk);
        if (!sock_tag_entry) {
                spin_unlock_bh(&sock_tag_list_lock);
                res = -EINVAL;
-               goto err_put;
+               return res;
        }
        /*
         * The socket already belongs to the current process
@@ -2415,20 +2423,26 @@ static int ctrl_cmd_untag(const char *input)
        BUG_ON(!tag_ref_entry);
        BUG_ON(tag_ref_entry->num_sock_tags <= 0);
        spin_lock_bh(&uid_tag_data_tree_lock);
+       if (kernel)
+               pid = sock_tag_entry->pid;
+       else
+               pid = current->tgid;
        pqd_entry = proc_qtu_data_tree_search(
-               &proc_qtu_data_tree, current->tgid);
+               &proc_qtu_data_tree, pid);
        /*
         * TODO: remove if, and start failing.
         * At first, we want to catch user-space code that is not
         * opening the /dev/xt_qtaguid.
         */
-       if (IS_ERR_OR_NULL(pqd_entry))
+       if (IS_ERR_OR_NULL(pqd_entry) || !sock_tag_entry->list.next) {
                pr_warn_once("qtaguid: %s(): "
                             "User space forgot to open /dev/xt_qtaguid? "
-                            "pid=%u tgid=%u uid=%u\n", __func__,
-                            current->pid, current->tgid, from_kuid(&init_user_ns, current_fsuid()));
-       else
+                            "pid=%u tgid=%u sk_pid=%u, uid=%u\n", __func__,
+                            current->pid, current->tgid, sock_tag_entry->pid,
+                            from_kuid(&init_user_ns, current_fsuid()));
+       } else {
                list_del(&sock_tag_entry->list);
+       }
        spin_unlock_bh(&uid_tag_data_tree_lock);
        /*
         * We don't free tag_ref from the utd_entry here,
@@ -2437,30 +2451,17 @@ static int ctrl_cmd_untag(const char *input)
        tag_ref_entry->num_sock_tags--;
        spin_unlock_bh(&sock_tag_list_lock);
        /*
-        * Release the sock_fd that was grabbed at tag time,
-        * and once more for the sockfd_lookup() here.
+        * Release the sock_fd that was grabbed at tag time.
         */
-       sockfd_put(sock_tag_entry->socket);
-       CT_DEBUG("qtaguid: ctrl_untag(%s): done. st@%p ...->f_count=%ld\n",
-                input, sock_tag_entry,
-                atomic_long_read(&el_socket->file->f_count) - 1);
-       sockfd_put(el_socket);
+       sock_put(sock_tag_entry->sk);
+       CT_DEBUG("qtaguid: done. st@%p ...->sk_refcnt=%d\n",
+                sock_tag_entry,
+                atomic_read(&el_socket->sk->sk_refcnt));
 
        kfree(sock_tag_entry);
        atomic64_inc(&qtu_events.sockets_untagged);
 
        return 0;
-
-err_put:
-       CT_DEBUG("qtaguid: ctrl_untag(%s): done. socket->...->f_count=%ld\n",
-                input, atomic_long_read(&el_socket->file->f_count) - 1);
-       /* Release the sock_fd that was grabbed by sockfd_lookup(). */
-       sockfd_put(el_socket);
-       return res;
-
-err:
-       CT_DEBUG("qtaguid: ctrl_untag(%s): done.\n", input);
-       return res;
 }
 
 static ssize_t qtaguid_ctrl_parse(const char *input, size_t count)

diff --git a/net/netfilter/xt_qtaguid_internal.h b/net/netfilter/xt_qtaguid_internal.h
index 6dc14a9c688966addc6ef949209fa70a70d00024..8178fbdfb0366b952a36e86138cfc1fa2a4a2bbd 100644 (file)
--- a/net/netfilter/xt_qtaguid_internal.h
+++ b/net/netfilter/xt_qtaguid_internal.h
@@ -256,8 +256,6 @@ struct iface_stat_work {
 struct sock_tag {
        struct rb_node sock_node;
        struct sock *sk;  /* Only used as a number, never dereferenced */
-       /* The socket is needed for sockfd_put() */
-       struct socket *socket;
        /* Used to associate with a given pid */
        struct list_head list;   /* in proc_qtu_data.sock_tag_list */
        pid_t pid;

diff --git a/net/netfilter/xt_qtaguid_print.c b/net/netfilter/xt_qtaguid_print.c
index f6a00a3520ed577218a503f8b0352d2b7f8e5094..2a7190d285e607cd523cbca4bce417fd12933dd5 100644 (file)
--- a/net/netfilter/xt_qtaguid_print.c
+++ b/net/netfilter/xt_qtaguid_print.c
@@ -24,7 +24,7 @@
 #include <linux/rbtree.h>
 #include <linux/slab.h>
 #include <linux/spinlock_types.h>
-
+#include <net/sock.h>
 
 #include "xt_qtaguid_internal.h"
 #include "xt_qtaguid_print.h"
@@ -237,10 +237,10 @@ char *pp_sock_tag(struct sock_tag *st)
        tag_str = pp_tag_t(&st->tag);
        res = kasprintf(GFP_ATOMIC, "sock_tag@%p{"
                        "sock_node=rb_node{...}, "
-                       "sk=%p socket=%p (f_count=%lu), list=list_head{...}, "
+                       "sk=%p (f_count=%d), list=list_head{...}, "
                        "pid=%u, tag=%s}",
-                       st, st->sk, st->socket, atomic_long_read(
-                               &st->socket->file->f_count),
+                       st, st->sk, atomic_read(
+                               &st->sk->sk_refcnt),
                        st->pid, tag_str);
        _bug_on_err_or_null(res);
        kfree(tag_str);