Re: [PATCH mptcp-net] mptcp: fix double-free on socket dismantle

[Mat Martineau <martineau@kernel.org>]

On Wed, 21 Feb 2024, Davide Caratti wrote:

> when MPTCP server accepts an incoming connection, it clones its listener
> socket. However, the pointer to 'inet_opt' for the new socket has the same
> value as the original one: as a consequence, on program exit it's possible
> to observe the following splat:
>
...
>
> Something similar (a refcount underflow) happens with CALIPSO/IPv6. Fix
> this by duplicating IP / IPv6 options after clone, so that
> ip{,6}_sock_destruct() doesn't end up freeing the same memory area twice.
>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/protocol.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)

Thanks Davide. Looks good to me, and the appropriate "fixes" tag goes all 
the way back to the beginning:

Fixes: cf7da0d66cc1 ("mptcp: Create SUBFLOW socket for incoming connections")

Reviewed-by: Mat Martineau <martineau@kernel.org>


>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 50dcba41b6ef..352334bda0e3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3202,8 +3202,50 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
>
> 	return (struct ipv6_pinfo *)(((u8 *)sk) + offset);
> }
> +
> +static void mptcp_copy_ip6_options(struct sock *newsk, const struct sock *sk)
> +{
> +	const struct ipv6_pinfo *np = inet6_sk(sk);
> +	struct ipv6_txoptions *opt;
> +	struct ipv6_pinfo *newnp;
> +
> +	newnp = inet6_sk(newsk);
> +
> +	rcu_read_lock();
> +	opt = rcu_dereference(np->opt);
> +	if (opt) {
> +		opt = ipv6_dup_options(newsk, opt);
> +		if (!opt)
> +			net_warn_ratelimited("%s: Failed to copy ip6 options\n", __func__);
> +	}
> +	RCU_INIT_POINTER(newnp->opt, opt);
> +	rcu_read_unlock();
> +}
> #endif
>
> +static void mptcp_copy_ip_options(struct sock *newsk, const struct sock *sk)
> +{
> +	struct ip_options_rcu *inet_opt, *newopt = NULL;
> +	const struct inet_sock *inet = inet_sk(sk);
> +	struct inet_sock *newinet;
> +
> +	newinet = inet_sk(newsk);
> +
> +	rcu_read_lock();
> +	inet_opt = rcu_dereference(inet->inet_opt);
> +	if (inet_opt) {
> +		newopt = sock_kmalloc(newsk, sizeof(*inet_opt) +
> +				      inet_opt->opt.optlen, GFP_ATOMIC);
> +		if (newopt)
> +			memcpy(newopt, inet_opt, sizeof(*inet_opt) +
> +			       inet_opt->opt.optlen);
> +		else
> +			net_warn_ratelimited("%s: Failed to copy ip options\n", __func__);
> +	}
> +	RCU_INIT_POINTER(newinet->inet_opt, newopt);
> +	rcu_read_unlock();
> +}
> +
> struct sock *mptcp_sk_clone_init(const struct sock *sk,
> 				 const struct mptcp_options_received *mp_opt,
> 				 struct sock *ssk,
> @@ -3224,6 +3266,13 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
>
> 	__mptcp_init_sock(nsk);
>
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +	if (nsk->sk_family == AF_INET6)
> +		mptcp_copy_ip6_options(nsk, sk);
> +	else
> +#endif
> +		mptcp_copy_ip_options(nsk, sk);
> +
> 	msk = mptcp_sk(nsk);
> 	WRITE_ONCE(msk->local_key, subflow_req->local_key);
> 	WRITE_ONCE(msk->token, subflow_req->token);
> -- 
> 2.43.0
>
>
>