* [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
@ 2021-05-06 17:24 Paolo Abeni
2021-05-06 20:11 ` Florian Westphal
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Paolo Abeni @ 2021-05-06 17:24 UTC (permalink / raw)
To: mptcp; +Cc: Christoph Paasch, Florian Westphal
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
2021-05-06 17:24 [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt() Paolo Abeni
@ 2021-05-06 20:11 ` Florian Westphal
2021-05-07 0:46 ` Mat Martineau
2021-05-07 15:41 ` Matthieu Baerts
2 siblings, 0 replies; 7+ messages in thread
From: Florian Westphal @ 2021-05-06 20:11 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Christoph Paasch, 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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
2021-05-06 17:24 [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt() Paolo Abeni
2021-05-06 20:11 ` Florian Westphal
@ 2021-05-07 0:46 ` Mat Martineau
2021-05-07 15:41 ` Matthieu Baerts
2 siblings, 0 replies; 7+ messages in thread
From: Mat Martineau @ 2021-05-07 0:46 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Christoph Paasch, Florian Westphal
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
2021-05-06 17:24 [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt() Paolo Abeni
2021-05-06 20:11 ` Florian Westphal
2021-05-07 0:46 ` Mat Martineau
@ 2021-05-07 15:41 ` Matthieu Baerts
2021-05-07 16:45 ` Mat Martineau
2 siblings, 1 reply; 7+ messages in thread
From: Matthieu Baerts @ 2021-05-07 15:41 UTC (permalink / raw)
To: Paolo Abeni, Florian Westphal, Mat Martineau; +Cc: mptcp
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
2021-05-07 15:41 ` Matthieu Baerts
@ 2021-05-07 16:45 ` Mat Martineau
2021-05-07 17:45 ` Matthieu Baerts
0 siblings, 1 reply; 7+ messages in thread
From: Mat Martineau @ 2021-05-07 16:45 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: Paolo Abeni, Florian Westphal, mptcp
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
2021-05-07 16:45 ` Mat Martineau
@ 2021-05-07 17:45 ` Matthieu Baerts
2021-05-07 19:00 ` Mat Martineau
0 siblings, 1 reply; 7+ messages in thread
From: Matthieu Baerts @ 2021-05-07 17:45 UTC (permalink / raw)
To: Mat Martineau; +Cc: Paolo Abeni, Florian Westphal, mptcp
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt()
2021-05-07 17:45 ` Matthieu Baerts
@ 2021-05-07 19:00 ` Mat Martineau
0 siblings, 0 replies; 7+ messages in thread
From: Mat Martineau @ 2021-05-07 19:00 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: Paolo Abeni, Florian Westphal, mptcp
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-07 19:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 17:24 [PATCH mptcp-net] mptcp: avoid OOB access in setsockopt() Paolo Abeni
2021-05-06 20:11 ` Florian Westphal
2021-05-07 0:46 ` Mat Martineau
2021-05-07 15:41 ` Matthieu Baerts
2021-05-07 16:45 ` Mat Martineau
2021-05-07 17:45 ` Matthieu Baerts
2021-05-07 19:00 ` Mat Martineau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.