[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Christoph Paasch <cpaasch at apple.com>]

[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]

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(a)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