[PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Paolo Abeni]

We can't use tcp_set_congestion_control() on an mptcp socket, as
such function can end-up accessing a tcp-specific field -
prior_ssthresh - causing an OOB access.

To allow propagating the correct ca algo on subflow, cache the ca
name at initialization time.

Additionally avoid overriding the user-selected CA (if any) at
clone time.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/182
Fixes: aa1fbd94e5c7 ("mptcp: sockopt: add TCP_CONGESTION and TCP_INFO")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
@Christoph: could u please give this a spin?
---
 net/mptcp/protocol.c | 14 +++++++++++---
 net/mptcp/protocol.h |  1 +
 net/mptcp/sockopt.c  |  4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 32a39774075b..627352c59416 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2437,8 +2437,6 @@ static int __mptcp_init_sock(struct sock *sk)
 	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_retransmit_timer, 0);
 	timer_setup(&sk->sk_timer, mptcp_timeout_timer, 0);
 
-	tcp_assign_congestion_control(sk);
-
 #if IS_ENABLED(CONFIG_KASAN)
 	sock_set_flag(sk, SOCK_RCU_FREE);
 #endif
@@ -2448,6 +2446,7 @@ static int __mptcp_init_sock(struct sock *sk)
 
 static int mptcp_init_sock(struct sock *sk)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct net *net = sock_net(sk);
 	int ret;
 
@@ -2465,6 +2464,16 @@ static int mptcp_init_sock(struct sock *sk)
 	if (ret)
 		return ret;
 
+	/* fetch the ca name; do it outside __mptcp_init_sock(), so that clone will
+	 * propagate the correct value
+	 */
+	tcp_assign_congestion_control(sk);
+	strcpy(mptcp_sk(sk)->ca_name, icsk->icsk_ca_ops->name);
+
+	/* no need to keep a reference to the oops, the name will suffice */
+	tcp_cleanup_congestion_control(sk);
+	icsk->icsk_ca_ops = NULL;
+
 	sk_sockets_allocated_inc(sk);
 	sk->sk_rcvbuf = sock_net(sk)->ipv4.sysctl_tcp_rmem[1];
 	sk->sk_sndbuf = sock_net(sk)->ipv4.sysctl_tcp_wmem[1];
@@ -2639,7 +2648,6 @@ static void __mptcp_destroy_sock(struct sock *sk)
 	sk_stream_kill_queues(sk);
 	xfrm_sk_free_policy(sk);
 
-	tcp_cleanup_congestion_control(sk);
 	sk_refcnt_debug_release(sk);
 	mptcp_dispose_initial_subflow(msk);
 	sock_put(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 98c735f237b4..868e878af526 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -263,6 +263,7 @@ struct mptcp_sock {
 	} rcvq_space;
 
 	u32 setsockopt_seq;
+	char		ca_name[TCP_CA_NAME_MAX];
 };
 
 #define mptcp_lock_sock(___sk, cb) do {					\
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 00d941b66c1e..a79798189599 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -547,7 +547,7 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
 	}
 
 	if (ret == 0)
-		tcp_set_congestion_control(sk, name, false, cap_net_admin);
+		strcpy(msk->ca_name, name);
 
 	release_sock(sk);
 	return ret;
@@ -705,7 +705,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
 	sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG));
 
 	if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops)
-		tcp_set_congestion_control(ssk, inet_csk(sk)->icsk_ca_ops->name, false, true);
+		tcp_set_congestion_control(ssk, msk->ca_name, false, true);
 }
 
 static void __mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk)
-- 
2.26.2

Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> We can't use tcp_set_congestion_control() on an mptcp socket, as
> such function can end-up accessing a tcp-specific field -
> prior_ssthresh - causing an OOB access.

Nice find.

> To allow propagating the correct ca algo on subflow, cache the ca
> name at initialization time.

Ok, this removes the problem of setting the right icsk_ca_ops on the
mptcp socket.

Thanks for fixing this!

Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Mat Martineau]

On Thu, 6 May 2021, Paolo Abeni wrote:

> We can't use tcp_set_congestion_control() on an mptcp socket, as
> such function can end-up accessing a tcp-specific field -
> prior_ssthresh - causing an OOB access.
>
> To allow propagating the correct ca algo on subflow, cache the ca
> name at initialization time.
>
> Additionally avoid overriding the user-selected CA (if any) at
> clone time.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/182
> Fixes: aa1fbd94e5c7 ("mptcp: sockopt: add TCP_CONGESTION and TCP_INFO")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> @Christoph: could u please give this a spin?
> ---
> net/mptcp/protocol.c | 14 +++++++++++---
> net/mptcp/protocol.h |  1 +
> net/mptcp/sockopt.c  |  4 ++--
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 32a39774075b..627352c59416 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2437,8 +2437,6 @@ static int __mptcp_init_sock(struct sock *sk)
> 	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_retransmit_timer, 0);
> 	timer_setup(&sk->sk_timer, mptcp_timeout_timer, 0);
>
> -	tcp_assign_congestion_control(sk);
> -
> #if IS_ENABLED(CONFIG_KASAN)
> 	sock_set_flag(sk, SOCK_RCU_FREE);
> #endif
> @@ -2448,6 +2446,7 @@ static int __mptcp_init_sock(struct sock *sk)
>
> static int mptcp_init_sock(struct sock *sk)
> {
> +	struct inet_connection_sock *icsk = inet_csk(sk);
> 	struct net *net = sock_net(sk);
> 	int ret;
>
> @@ -2465,6 +2464,16 @@ static int mptcp_init_sock(struct sock *sk)
> 	if (ret)
> 		return ret;
>
> +	/* fetch the ca name; do it outside __mptcp_init_sock(), so that clone will
> +	 * propagate the correct value
> +	 */
> +	tcp_assign_congestion_control(sk);
> +	strcpy(mptcp_sk(sk)->ca_name, icsk->icsk_ca_ops->name);
> +
> +	/* no need to keep a reference to the oops, the name will suffice */

Given that "oops" is a loaded word in the kernel world, I think this typo 
is worth fixing!

I will also run this in syzkaller in case Christoph can't get to it.


Mat


> +	tcp_cleanup_congestion_control(sk);
> +	icsk->icsk_ca_ops = NULL;
> +
> 	sk_sockets_allocated_inc(sk);
> 	sk->sk_rcvbuf = sock_net(sk)->ipv4.sysctl_tcp_rmem[1];
> 	sk->sk_sndbuf = sock_net(sk)->ipv4.sysctl_tcp_wmem[1];
> @@ -2639,7 +2648,6 @@ static void __mptcp_destroy_sock(struct sock *sk)
> 	sk_stream_kill_queues(sk);
> 	xfrm_sk_free_policy(sk);
>
> -	tcp_cleanup_congestion_control(sk);
> 	sk_refcnt_debug_release(sk);
> 	mptcp_dispose_initial_subflow(msk);
> 	sock_put(sk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 98c735f237b4..868e878af526 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -263,6 +263,7 @@ struct mptcp_sock {
> 	} rcvq_space;
>
> 	u32 setsockopt_seq;
> +	char		ca_name[TCP_CA_NAME_MAX];
> };
>
> #define mptcp_lock_sock(___sk, cb) do {					\
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 00d941b66c1e..a79798189599 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -547,7 +547,7 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
> 	}
>
> 	if (ret == 0)
> -		tcp_set_congestion_control(sk, name, false, cap_net_admin);
> +		strcpy(msk->ca_name, name);
>
> 	release_sock(sk);
> 	return ret;
> @@ -705,7 +705,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
> 	sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG));
>
> 	if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops)
> -		tcp_set_congestion_control(ssk, inet_csk(sk)->icsk_ca_ops->name, false, true);
> +		tcp_set_congestion_control(ssk, msk->ca_name, false, true);
> }
>
> static void __mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk)
> -- 
> 2.26.2
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Matthieu Baerts]

Hi Paolo, Florian, Mat,

On 06/05/2021 19:24, Paolo Abeni wrote:
> We can't use tcp_set_congestion_control() on an mptcp socket, as
> such function can end-up accessing a tcp-specific field -
> prior_ssthresh - causing an OOB access.
> 
> To allow propagating the correct ca algo on subflow, cache the ca
> name at initialization time.
> 
> Additionally avoid overriding the user-selected CA (if any) at
> clone time.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/182
> Fixes: aa1fbd94e5c7 ("mptcp: sockopt: add TCP_CONGESTION and TCP_INFO")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the patch and the review!

I just applied the patch with Florian's Ack, a fix for the typo spot by
Mat but not RvB tag for Mat (@Mat: I guess you are OK with the patch but
it is unclear :) )

- 20744dcd6f36: mptcp: avoid OOB access in setsockopt()
- c69b45723290: conflict in t/DO-NOT-MERGE-mptcp-use-kmalloc-on-kasan-build
- Results: 37818f5e301f..837dc82fa10b

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210507T154124
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210507T154124

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Mat Martineau]

On Fri, 7 May 2021, Matthieu Baerts wrote:

> Hi Paolo, Florian, Mat,
>
> On 06/05/2021 19:24, Paolo Abeni wrote:
>> We can't use tcp_set_congestion_control() on an mptcp socket, as
>> such function can end-up accessing a tcp-specific field -
>> prior_ssthresh - causing an OOB access.
>>
>> To allow propagating the correct ca algo on subflow, cache the ca
>> name at initialization time.
>>
>> Additionally avoid overriding the user-selected CA (if any) at
>> clone time.
>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/182
>> Fixes: aa1fbd94e5c7 ("mptcp: sockopt: add TCP_CONGESTION and TCP_INFO")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> Thank you for the patch and the review!
>
> I just applied the patch with Florian's Ack, a fix for the typo spot by
> Mat but not RvB tag for Mat (@Mat: I guess you are OK with the patch but
> it is unclear :) )
>
> - 20744dcd6f36: mptcp: avoid OOB access in setsockopt()
> - c69b45723290: conflict in t/DO-NOT-MERGE-mptcp-use-kmalloc-on-kasan-build
> - Results: 37818f5e301f..837dc82fa10b
>
> Builds and tests are now in progress:
>
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210507T154124
> https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210507T154124
>

Hi Matthieu -

I ran syzkaller for a while and didn't see this crash (but without a 
reproducer I don't know how definitive that is.)

You can add my tag:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Matthieu Baerts]

Hi Mat,

On 07/05/2021 18:45, Mat Martineau wrote:
> I ran syzkaller for a while and didn't see this crash (but without a
> reproducer I don't know how definitive that is.)

Thank you for having started the tests!

I think Christoph said he quickly got the warning after having
re-started syzkaller yesterday at the beginning of the meeting.

Do you see the same issue without the patch?

But I guess we can be quite confident here that the bug is fixed :)

> You can add my tag:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Done!

- 983511e46a4f: tg:msg: add Mat's RvB tag
- Results: d76b60f54174..860b49961672

Now available in the export branch and export/20210507T174457 tag

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()

[Mat Martineau]

On Fri, 7 May 2021, Matthieu Baerts wrote:

> Hi Mat,
>
> On 07/05/2021 18:45, Mat Martineau wrote:
>> I ran syzkaller for a while and didn't see this crash (but without a
>> reproducer I don't know how definitive that is.)
>
> Thank you for having started the tests!
>
> I think Christoph said he quickly got the warning after having
> re-started syzkaller yesterday at the beginning of the meeting.
>
> Do you see the same issue without the patch?
>

I don't see the issue even without the patch. Code I'm running for that is 
net/master with the "use kmalloc on kasan build" patch from our export 
branch.

> But I guess we can be quite confident here that the bug is fixed :)
>
>> You can add my tag:
>>
>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
> Done!
>
> - 983511e46a4f: tg:msg: add Mat's RvB tag
> - Results: d76b60f54174..860b49961672
>
> Now available in the export branch and export/20210507T174457 tag
>

--
Mat Martineau
Intel