mptcp: Correctly set DATA_FIN timeout when number of retransmits is large

[PATCH mptcp-net] mptcp: Correctly set DATA_FIN timeout when number of retransmits is large

Posted by Mat Martineau 3 years, 12 months ago

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

Re: [PATCH mptcp-net] mptcp: Correctly set DATA_FIN timeout when number of retransmits is large

Posted by Paolo Abeni 3 years, 11 months ago

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)

Re: [PATCH mptcp-net] mptcp: Correctly set DATA_FIN timeout when number of retransmits is large

Posted by Matthieu Baerts 3 years, 11 months ago

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