Message ID | 79b377c2cb932a86f0fd1a17f814064dd1ad7015.1620321074.git.pabeni@redhat.com |
---|---|
State | Accepted, archived |
Commit | 20744dcd6f368b15379fdb74ba3546530331fa41 |
Delegated to: | Matthieu Baerts |
Headers | show |
Series | [mptcp-net] mptcp: avoid OOB access in setsockopt() | expand |
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!
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
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
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
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
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
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)
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(-)