Re: [PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2023-10-09 at 14:02 +0800, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), if there's
> another subflow available, it's better to avoid resetting it.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30e0c29ae0a4..6346a164ed66 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		goto out_release;
>  	}
>  
> -	dispose_it = msk->free_first || ssk != msk->first;
> +	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
>  	if (dispose_it)
>  		list_del(&subflow->node);

I'm sorry for the late feedback.

We can't do the above, Syzkaller already hit us in the past while
attempting that thing.

We need to be msk->first != NULL and a valid, derefereciable pointer up
to mptcp_destroy() - that is, up to when the msk socket is freed -
otherwise we will hit a number of UaF in many places.

Note that claring msk->first here, and add check for 'msk->first !=
NULL' before every msk access, will not save us, sometimes msk->first
is tested without the msk socket lock. 

I think there are 2 options here:

- the caller (PM NL/PM userspace) could explicitly avoid calling
mptcp_close_ssk() when removing subflow 0 (just shut it down)

- Add a new flags MPTCP_NO_DISCONNECT, and let the caller pass it here.
  When MPTCP_NO_DISCONNECT is set, we invoke tcp_shutdown() instead of
tcp_disconnect()

Both options are actually quite similar, the 2nd is possibly the
cleanest.

Note that looking here for 'this is the last subflow' and avoid the
disconnect otherwise, does not look correct/safe: we want to invoke
tcp_disconnect() even when first is _not_ the last subflow and we are
reaching mptcp_close_ssk() from a different caller - e.g. from
mptcp_disconnect()

/P