| Message ID | a54e1b917f151b550951a0bad0b69a4924885ce1.1603211002.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | 51c3efe449ecb1d0c39ea86f66539fd3a1e0cb97 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [net-next] Squash-to: "mptcp: refactor shutdown and close" | expand |
On Tue, 20 Oct 2020, Paolo Abeni wrote: > we must release the ssk socket only after orphaning it, > or we may hit UaF in the receive path. > > Beyond the error critical code path reported by syzkaller, > there is also the scenario of a subflow closed by the PM. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > Should fix 101 by I actually tested this only vs self-tests Looks fine in terms of code review. I will work on verifying it with syzkaller tomorrow unless I hear something from Christoph. Mat > --- > net/mptcp/protocol.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index a32c27fe525c..2240fd370014 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1746,16 +1746,22 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk) > void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, > struct mptcp_subflow_context *subflow) > { > - struct socket *sock = READ_ONCE(ssk->sk_socket); > + bool dispose_socket = false; > + struct socket *sock; > > list_del(&subflow->node); > > - /* outgoing subflow */ > - if (sock && sock != sk->sk_socket) > - iput(SOCK_INODE(sock)); > - > lock_sock(ssk); > > + /* if we are invoked by the msk cleanup code, the subflow is > + * already orphaned > + */ > + sock = ssk->sk_socket; > + if (sock) { > + dispose_socket = sock != sk->sk_socket; > + sock_orphan(ssk); > + } > + > /* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops > * the ssk has been already destroyed, we just need to release the > * reference owned by msk; > @@ -1771,6 +1777,9 @@ void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, > __sock_put(ssk); > } > release_sock(ssk); > + if (dispose_socket) > + iput(SOCK_INODE(sock)); > + > sock_put(ssk); > } > > @@ -2165,15 +2174,20 @@ static void mptcp_close(struct sock *sk, long timeout) > inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32; > list_for_each_entry(subflow, &mptcp_sk(sk)->conn_list, node) { > struct sock *ssk = mptcp_subflow_tcp_sock(subflow); > - struct socket *sock = READ_ONCE(ssk->sk_socket); > - bool slow; > - > - if (sock && sock != sk->sk_socket) > - iput(SOCK_INODE(sock)); > + bool slow, dispose_socket; > + struct socket *sock; > > slow = lock_sock_fast(ssk); > + sock = ssk->sk_socket; > + dispose_socket = sock && sock != sk->sk_socket; > sock_orphan(ssk); > unlock_sock_fast(ssk, slow); > + > + /* for the outgoing subflows we additionally need to free > + * the associated socket > + */ > + if (dispose_socket) > + iput(SOCK_INODE(sock)); > } > sock_orphan(sk); > > -- > 2.26.2 -- Mat Martineau Intel
On 10/20/20 - 20:51, Mat Martineau wrote: > On Tue, 20 Oct 2020, Paolo Abeni wrote: > > > we must release the ssk socket only after orphaning it, > > or we may hit UaF in the receive path. > > > > Beyond the error critical code path reported by syzkaller, > > there is also the scenario of a subflow closed by the PM. > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > Should fix 101 by I actually tested this only vs self-tests > > Looks fine in terms of code review. I will work on verifying it with > syzkaller tomorrow unless I hear something from Christoph. I'm having some problems with my syzkaller, so I can't test right now. Christoph > > > Mat > > > > --- > > net/mptcp/protocol.c | 34 ++++++++++++++++++++++++---------- > > 1 file changed, 24 insertions(+), 10 deletions(-) > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index a32c27fe525c..2240fd370014 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -1746,16 +1746,22 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk) > > void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, > > struct mptcp_subflow_context *subflow) > > { > > - struct socket *sock = READ_ONCE(ssk->sk_socket); > > + bool dispose_socket = false; > > + struct socket *sock; > > > > list_del(&subflow->node); > > > > - /* outgoing subflow */ > > - if (sock && sock != sk->sk_socket) > > - iput(SOCK_INODE(sock)); > > - > > lock_sock(ssk); > > > > + /* if we are invoked by the msk cleanup code, the subflow is > > + * already orphaned > > + */ > > + sock = ssk->sk_socket; > > + if (sock) { > > + dispose_socket = sock != sk->sk_socket; > > + sock_orphan(ssk); > > + } > > + > > /* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops > > * the ssk has been already destroyed, we just need to release the > > * reference owned by msk; > > @@ -1771,6 +1777,9 @@ void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, > > __sock_put(ssk); > > } > > release_sock(ssk); > > + if (dispose_socket) > > + iput(SOCK_INODE(sock)); > > + > > sock_put(ssk); > > } > > > > @@ -2165,15 +2174,20 @@ static void mptcp_close(struct sock *sk, long timeout) > > inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32; > > list_for_each_entry(subflow, &mptcp_sk(sk)->conn_list, node) { > > struct sock *ssk = mptcp_subflow_tcp_sock(subflow); > > - struct socket *sock = READ_ONCE(ssk->sk_socket); > > - bool slow; > > - > > - if (sock && sock != sk->sk_socket) > > - iput(SOCK_INODE(sock)); > > + bool slow, dispose_socket; > > + struct socket *sock; > > > > slow = lock_sock_fast(ssk); > > + sock = ssk->sk_socket; > > + dispose_socket = sock && sock != sk->sk_socket; > > sock_orphan(ssk); > > unlock_sock_fast(ssk, slow); > > + > > + /* for the outgoing subflows we additionally need to free > > + * the associated socket > > + */ > > + if (dispose_socket) > > + iput(SOCK_INODE(sock)); > > } > > sock_orphan(sk); > > > > -- > > 2.26.2 > > -- > Mat Martineau > Intel
Hi Paolo, Mat, On 20/10/2020 18:25, Paolo Abeni wrote: > we must release the ssk socket only after orphaning it, > or we may hit UaF in the receive path. > > Beyond the error critical code path reported by syzkaller, > there is also the scenario of a subflow closed by the PM. Thank you for this patch and the review! - 51c3efe449ec: "squashed" in "mptcp: refactor shutdown and close" - Results: da50c705808b..62d3c2d391fa Tests + export are going to be started soon! Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index a32c27fe525c..2240fd370014 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1746,16 +1746,22 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk) void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, struct mptcp_subflow_context *subflow) { - struct socket *sock = READ_ONCE(ssk->sk_socket); + bool dispose_socket = false; + struct socket *sock; list_del(&subflow->node); - /* outgoing subflow */ - if (sock && sock != sk->sk_socket) - iput(SOCK_INODE(sock)); - lock_sock(ssk); + /* if we are invoked by the msk cleanup code, the subflow is + * already orphaned + */ + sock = ssk->sk_socket; + if (sock) { + dispose_socket = sock != sk->sk_socket; + sock_orphan(ssk); + } + /* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops * the ssk has been already destroyed, we just need to release the * reference owned by msk; @@ -1771,6 +1777,9 @@ void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, __sock_put(ssk); } release_sock(ssk); + if (dispose_socket) + iput(SOCK_INODE(sock)); + sock_put(ssk); } @@ -2165,15 +2174,20 @@ static void mptcp_close(struct sock *sk, long timeout) inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32; list_for_each_entry(subflow, &mptcp_sk(sk)->conn_list, node) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); - struct socket *sock = READ_ONCE(ssk->sk_socket); - bool slow; - - if (sock && sock != sk->sk_socket) - iput(SOCK_INODE(sock)); + bool slow, dispose_socket; + struct socket *sock; slow = lock_sock_fast(ssk); + sock = ssk->sk_socket; + dispose_socket = sock && sock != sk->sk_socket; sock_orphan(ssk); unlock_sock_fast(ssk, slow); + + /* for the outgoing subflows we additionally need to free + * the associated socket + */ + if (dispose_socket) + iput(SOCK_INODE(sock)); } sock_orphan(sk);
we must release the ssk socket only after orphaning it, or we may hit UaF in the receive path. Beyond the error critical code path reported by syzkaller, there is also the scenario of a subflow closed by the PM. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- Should fix 101 by I actually tested this only vs self-tests --- net/mptcp/protocol.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)