ANDROID: netfilter: xt_qtaguid: 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/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index eeee221..da863cf 100644
--- 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 <linux/uaccess.h>
@@ -422,6 +423,9 @@
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 caba267..a7f4d8b 100644
--- a/net/netfilter/xt_qtaguid.c
+++ b/net/netfilter/xt_qtaguid.c
@@ -321,7 +321,7 @@
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);
}
}
@@ -1922,12 +1922,12 @@
{
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",
@@ -1936,13 +1936,13 @@
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 "
@@ -2238,8 +2238,8 @@
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);
@@ -2283,16 +2283,9 @@
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));
@@ -2312,8 +2305,12 @@
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);
@@ -2340,10 +2337,11 @@
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:
@@ -2351,8 +2349,8 @@
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;
@@ -2368,17 +2366,13 @@
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) {
@@ -2386,17 +2380,31 @@
" 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
@@ -2408,20 +2416,26 @@
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,
@@ -2430,30 +2444,17 @@
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 6dc14a9..8178fbd 100644
--- a/net/netfilter/xt_qtaguid_internal.h
+++ b/net/netfilter/xt_qtaguid_internal.h
@@ -256,8 +256,6 @@
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 f6a00a3..2a7190d 100644
--- 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 @@
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);