| Message ID | 3937b1a365f3f679c4f09887ba372d310af52f48.1591652866.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: more token follow-up | expand |
On Mon, 2020-06-08 at 23:51 +0200, Paolo Abeni wrote: > Move token creation into stream_connect(). This clean > the code a bit reducing the test in critical path and > plug a race: at shutdown time, subflow_rebuild_header() > can be called after the msk token is removed, leaving > a refecen to the to-be destroyed socket into the > token container with later UaF. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> hello Paolo, this patch fixes the kasan UAF and the GPF crash I saw yesterday on my test environments. See below: > --- > net/mptcp/protocol.c | 6 +++++- > net/mptcp/subflow.c | 10 ++-------- > 2 files changed, 7 insertions(+), 9 deletions(-) [...] > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index fcb9ca9a9dce..2d30fafa1aed 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c [...] > @@ -251,7 +245,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb) > subflow->remote_nonce = mp_opt.nonce; > pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u", subflow, > subflow->thmac, subflow->remote_nonce); > - } else if (subflow->request_mptcp) { > + } else { > tp->is_mptcp = 0; > } the above hunk does not apply because the lower context is different (it has been changed wityh the fallback rework). I reworked it like this: ------------------------ >8 ------------------------ @@ -252,7 +246,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb) subflow->remote_nonce = mp_opt.nonce; pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u", subflow, subflow->thmac, subflow->remote_nonce); - } else if (subflow->request_mptcp) { + } else { mptcp_do_fallback(sk); pr_fallback(mptcp_sk(subflow->conn)); MPTCP_INC_STATS(sock_net(sk), ------------------------ >8 ------------------------ During tests, I saw a sporadic packetdrill failure (one occurrence) here: /root/packetdrill/gtests/net/mptcp/mp_capable/v1_connect_tcpfallback_flagB.pkt and here: /root/packetdrill/gtests/net/mptcp/mp_capable/v1_bind_tcpfallback_wrongver.pkt but I'm unsure if this is really a problem related to this patch. I'm quite sure that it's something recent, because tests in the 'mp_capable' subfolder have been passing wothout problems since a long time.
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 99c019879833..be09fd525f8f 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1800,6 +1800,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, int addr_len, int flags) { struct mptcp_sock *msk = mptcp_sk(sock->sk); + struct mptcp_subflow_context *subflow; struct socket *ssock; int err; @@ -1819,13 +1820,16 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, goto unlock; } + subflow = mptcp_subflow_ctx(ssock->sk); #ifdef CONFIG_TCP_MD5SIG /* no MPTCP if MD5SIG is enabled on this socket or we may run out of * TCP option space. */ if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info)) - mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0; + subflow->request_mptcp = 0; #endif + if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) + subflow->request_mptcp = 0; do_connect: err = ssock->ops->connect(ssock, uaddr, addr_len, flags); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index fcb9ca9a9dce..2d30fafa1aed 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -34,13 +34,7 @@ static int subflow_rebuild_header(struct sock *sk) struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); int local_id; - if (subflow->request_mptcp && sk_unhashed(subflow->conn)) { - pr_debug("subflow=%p", sk); - if (mptcp_token_new_connect(sk)) { - subflow->mp_capable = 0; - goto out; - } - } else if (subflow->request_join && !subflow->local_nonce) { + if (subflow->request_join && !subflow->local_nonce) { struct mptcp_sock *msk = (struct mptcp_sock *)subflow->conn; pr_debug("subflow=%p", sk); @@ -251,7 +245,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb) subflow->remote_nonce = mp_opt.nonce; pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u", subflow, subflow->thmac, subflow->remote_nonce); - } else if (subflow->request_mptcp) { + } else { tp->is_mptcp = 0; }
Move token creation into stream_connect(). This clean the code a bit reducing the test in critical path and plug a race: at shutdown time, subflow_rebuild_header() can be called after the msk token is removed, leaving a refecen to the to-be destroyed socket into the token container with later UaF. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 6 +++++- net/mptcp/subflow.c | 10 ++-------- 2 files changed, 7 insertions(+), 9 deletions(-)