| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 17 Jun 2019 15:58:04 -0700
From: Mat Martineau <mathew.j.martineau@...ux.intel.com>
To: edumazet@...gle.com, netdev@...r.kernel.org
Cc: Florian Westphal <fw@...len.de>, cpaasch@...le.com,
pabeni@...hat.com, peter.krystad@...ux.intel.com,
dcaratti@...hat.com, matthieu.baerts@...sares.net
Subject: [RFC PATCH net-next 29/33] mptcp: accept: don't leak mptcp socket structure
From: Florian Westphal <fw@...len.de>
accept() is supposed to prepare and return a 'struct sock'.
The caller holds a new inode/socket, and will associate the
returned sock with it.
mptcp_accept however will allocate it via 'struct socket', then
returns socket->sk.
This then leaks the outer socket struct inode returned by sock_create():
unreferenced object 0xffff88810512e8c0 (size 936):
comm "mptcp_connect", [..]
backtrace:
[<00000000872561ba>] alloc_inode+0x35/0xe0
[<00000000646e04ed>] new_inode_pseudo+0x12/0x80
[<00000000e2e77036>] sock_alloc+0x26/0x100
[<00000000870a8688>] __sock_create+0x8f/0x3c0
[<000000000558b3fa>] mptcp_accept+0x140/0x530
[<0000000044718d60>] inet_accept+0xac/0x470
[<00000000da8f3979>] mptcp_stream_accept+0x62/0xa0
[<00000000c9010499>] __sys_accept4+0x228/0x3c0
To fix this, make several (unfortunately, intrusive) changes:
1. Instead of allocating a new mptcp socket, clone the
mptcp listen socket socket->sk.
This gives us a mptcp sock without the inode container.
We return this sock struct to the caller, the caller will then
complete creation of the mptcp socket.
2. For the 'compat' (old tcp, not mp capable) case, return the
tcp socket directly and release the socket coming from
kernel_accept().
We can use mptcp_getname() to override the socket->ops to
tcp. This will make mptcp_accept work like tcp accept, and
mptcp won't be involved anymore for the particular socket.
Signed-off-by: Florian Westphal <fw@...len.de>
---
net/mptcp/protocol.c | 69 +++++++++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 23 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3fb0f3163743..8c7b0f39394e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -617,9 +617,9 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
{
struct mptcp_sock *msk = mptcp_sk(sk);
struct socket *listener = msk->subflow;
- struct socket *new_sock;
- struct socket *new_mptcp_sock;
struct subflow_context *subflow;
+ struct socket *new_sock;
+ struct sock *newsk;
pr_debug("msk=%p, listener=%p", msk, subflow_ctx(listener->sk));
*err = kernel_accept(listener, &new_sock, flags);
@@ -629,28 +629,31 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
subflow = subflow_ctx(new_sock->sk);
pr_debug("msk=%p, new subflow=%p, ", msk, subflow);
- *err = sock_create(PF_INET, SOCK_STREAM, IPPROTO_MPTCP,
- &new_mptcp_sock);
- if (*err < 0) {
- kernel_sock_shutdown(new_sock, SHUT_RDWR);
- sock_release(new_sock);
- return NULL;
- }
-
- msk = mptcp_sk(new_mptcp_sock->sk);
- pr_debug("new msk=%p", msk);
-
if (subflow->mp_capable) {
+ struct sock *new_mptcp_sock;
u64 ack_seq;
+ local_bh_disable();
+ new_mptcp_sock = sk_clone_lock(sk, GFP_ATOMIC);
+ if (!new_mptcp_sock) {
+ *err = -ENOBUFS;
+ local_bh_enable();
+ kernel_sock_shutdown(new_sock, SHUT_RDWR);
+ sock_release(new_sock);
+ return NULL;
+ }
+
+ mptcp_init_sock(new_mptcp_sock);
+
+ msk = mptcp_sk(new_mptcp_sock);
msk->remote_key = subflow->remote_key;
msk->local_key = subflow->local_key;
msk->token = subflow->token;
- token_update_accept(new_sock->sk, new_mptcp_sock->sk);
- spin_lock_bh(&msk->conn_list_lock);
+ token_update_accept(new_sock->sk, new_mptcp_sock);
+ spin_lock(&msk->conn_list_lock);
list_add_rcu(&subflow->node, &msk->conn_list);
msk->subflow = NULL;
- spin_unlock_bh(&msk->conn_list_lock);
+ spin_unlock(&msk->conn_list_lock);
crypto_key_sha1(msk->remote_key, NULL, &ack_seq);
msk->write_seq = subflow->idsn + 1;
@@ -659,14 +662,20 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
subflow->map_seq = ack_seq;
subflow->map_subflow_seq = 1;
subflow->rel_write_seq = 1;
- subflow->conn = new_mptcp_sock->sk;
subflow->tcp_sock = new_sock;
+ newsk = new_mptcp_sock;
+ subflow->conn = new_mptcp_sock;
+ bh_unlock_sock(new_mptcp_sock);
+ local_bh_enable();
+ inet_sk_state_store(newsk, TCP_ESTABLISHED);
} else {
- msk->subflow = new_sock;
+ newsk = new_sock->sk;
+ tcp_sk(newsk)->is_mptcp = 0;
+ new_sock->sk = NULL;
+ sock_release(new_sock);
}
- inet_sk_state_store(new_mptcp_sock->sk, TCP_ESTABLISHED);
- return new_mptcp_sock->sk;
+ return newsk;
}
static void mptcp_destroy(struct sock *sk)
@@ -854,6 +863,18 @@ static int mptcp_getname(struct socket *sock, struct sockaddr *uaddr,
pr_debug("msk=%p", msk);
+ if (sock->sk->sk_prot == &tcp_prot) {
+ /* we are being invoked from __sys_accept4, after
+ * mptcp_accept() has just accepted a non-mp-capable
+ * flow: sk is a tcp_sk, not an mptcp one.
+ *
+ * Hand the socket over to tcp so all further socket ops
+ * bypass mptcp.
+ */
+ sock->ops = &inet_stream_ops;
+ return inet_getname(sock, uaddr, peer);
+ }
+
if (msk->subflow) {
pr_debug("subflow=%p", msk->subflow->sk);
return inet_getname(msk->subflow, uaddr, peer);
@@ -967,10 +988,12 @@ static int mptcp_shutdown(struct socket *sock, int how)
lock_sock(sock->sk);
rcu_read_lock();
list_for_each_entry_rcu(subflow, &msk->conn_list, node) {
- pr_debug("conn_list->subflow=%p", subflow);
+ struct socket *tcp_socket;
+
+ tcp_socket = mptcp_subflow_tcp_socket(subflow);
rcu_read_unlock();
- ret = kernel_sock_shutdown(mptcp_subflow_tcp_socket(subflow),
- how);
+ pr_debug("conn_list->subflow=%p", subflow);
+ ret = kernel_sock_shutdown(tcp_socket, how);
rcu_read_lock();
}
rcu_read_unlock();
--
2.22.0
Powered by blists - more mailing lists