net/mptcp/protocol.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Syzkaller with UBSAN uncovered a scenario where a large number of
DATA_FIN retransmits caused a shift-out-of-bounds in the DATA_FIN
timeout calculation:
================================================================================
UBSAN: shift-out-of-bounds in net/mptcp/protocol.c:470:29
shift exponent 32 is too large for 32-bit type 'unsigned int'
CPU: 1 PID: 13059 Comm: kworker/1:0 Not tainted 5.17.0-rc2-00630-g5fbf21c90c60 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
Workqueue: events mptcp_worker
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
ubsan_epilogue+0xb/0x5a lib/ubsan.c:151
__ubsan_handle_shift_out_of_bounds.cold+0xb2/0x20e lib/ubsan.c:330
mptcp_set_datafin_timeout net/mptcp/protocol.c:470 [inline]
__mptcp_retrans.cold+0x72/0x77 net/mptcp/protocol.c:2445
mptcp_worker+0x58a/0xa70 net/mptcp/protocol.c:2528
process_one_work+0x9df/0x16d0 kernel/workqueue.c:2307
worker_thread+0x95/0xe10 kernel/workqueue.c:2454
kthread+0x2f4/0x3b0 kernel/kthread.c:377
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
</TASK>
================================================================================
This change limits the maximum timeout by limiting the size of the
shift, which keeps all intermediate values in-bounds.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/259
Fixes: 6477dd39e62c ("mptcp: Retransmit DATA_FIN")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
net/mptcp/protocol.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3324e1c61576..a4171236091a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -466,9 +466,12 @@ static bool mptcp_pending_data_fin(struct sock *sk, u64 *seq)
static void mptcp_set_datafin_timeout(const struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
+ u32 retransmits;
- mptcp_sk(sk)->timer_ival = min(TCP_RTO_MAX,
- TCP_RTO_MIN << icsk->icsk_retransmits);
+ retransmits = min_t(u32, icsk->icsk_retransmits,
+ ilog2(TCP_RTO_MAX / TCP_RTO_MIN));
+
+ mptcp_sk(sk)->timer_ival = TCP_RTO_MIN << retransmits;
}
static void __mptcp_set_timeout(struct sock *sk, long tout)
base-commit: d53f81f7e645a73fae92f7d44076cdd6b7f8501c
--
2.35.1
On Fri, 2022-02-11 at 17:04 -0800, Mat Martineau wrote: > Syzkaller with UBSAN uncovered a scenario where a large number of > DATA_FIN retransmits caused a shift-out-of-bounds in the DATA_FIN > timeout calculation: > > ================================================================================ > UBSAN: shift-out-of-bounds in net/mptcp/protocol.c:470:29 > shift exponent 32 is too large for 32-bit type 'unsigned int' > CPU: 1 PID: 13059 Comm: kworker/1:0 Not tainted 5.17.0-rc2-00630-g5fbf21c90c60 #1 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014 > Workqueue: events mptcp_worker > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > ubsan_epilogue+0xb/0x5a lib/ubsan.c:151 > __ubsan_handle_shift_out_of_bounds.cold+0xb2/0x20e lib/ubsan.c:330 > mptcp_set_datafin_timeout net/mptcp/protocol.c:470 [inline] > __mptcp_retrans.cold+0x72/0x77 net/mptcp/protocol.c:2445 > mptcp_worker+0x58a/0xa70 net/mptcp/protocol.c:2528 > process_one_work+0x9df/0x16d0 kernel/workqueue.c:2307 > worker_thread+0x95/0xe10 kernel/workqueue.c:2454 > kthread+0x2f4/0x3b0 kernel/kthread.c:377 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 > </TASK> > ================================================================================ > > This change limits the maximum timeout by limiting the size of the > shift, which keeps all intermediate values in-bounds. > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/259 > Fixes: 6477dd39e62c ("mptcp: Retransmit DATA_FIN") > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > --- > net/mptcp/protocol.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 3324e1c61576..a4171236091a 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -466,9 +466,12 @@ static bool mptcp_pending_data_fin(struct sock *sk, u64 *seq) > static void mptcp_set_datafin_timeout(const struct sock *sk) > { > struct inet_connection_sock *icsk = inet_csk(sk); > + u32 retransmits; > > - mptcp_sk(sk)->timer_ival = min(TCP_RTO_MAX, > - TCP_RTO_MIN << icsk->icsk_retransmits); > + retransmits = min_t(u32, icsk->icsk_retransmits, > + ilog2(TCP_RTO_MAX / TCP_RTO_MIN)); > + > + mptcp_sk(sk)->timer_ival = TCP_RTO_MIN << retransmits; > } > > static void __mptcp_set_timeout(struct sock *sk, long tout) LGTM! Acked-by: Paolo Abeni <pabeni@redhat.com> (I think the the suggested-by tag could be dropped here)
Hi Mat, Paolo, On 17/02/2022 22:37, Paolo Abeni wrote: > On Fri, 2022-02-11 at 17:04 -0800, Mat Martineau wrote: >> Syzkaller with UBSAN uncovered a scenario where a large number of >> DATA_FIN retransmits caused a shift-out-of-bounds in the DATA_FIN >> timeout calculation: Thank you for the patch and the review! Now in our tree (fixes for -net) with Paolo's ACK: - 99aa4cb37fa4: mptcp: Correctly set DATA_FIN timeout when number of retransmits is large - Results: 4496c31e7f0d..52f48b4dbde0 > LGTM! > > Acked-by: Paolo Abeni <pabeni@redhat.com> > > (I think the the suggested-by tag could be dropped here) I left it there, I guess we can have the two, no? But Mat, feel free to remove it if you think it would be better without it :) Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220219T173647 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
© 2016 - 2024 Red Hat, Inc.