1. Patchew
  2. MPTCP
  3. View series

[PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout_timer

Dmitry Kandybka posted 1 patch 1 week, 6 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20241107103657.1560536-1-d.kandybka@gmail.com
net/mptcp/protocol.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout_timer
Posted by Dmitry Kandybka 1 week, 6 days ago 
In 'mptcp_reset_tout_timer', promote 'probe_timestamp' to unsigned long
to avoid possible integer overflow. Compile tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Kandybka <d.kandybka@gmail.com>
---
 net/mptcp/protocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e978e05ec8d1..ff2b8a2bfe18 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2722,8 +2722,8 @@ void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout)
 	if (!fail_tout && !inet_csk(sk)->icsk_mtup.probe_timestamp)
 		return;
 
-	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies +
-			mptcp_close_timeout(sk);
+	close_timeout = (unsigned long)inet_csk(sk)->icsk_mtup.probe_timestamp -
+			tcp_jiffies32 + jiffies + mptcp_close_timeout(sk);
 
 	/* the close timeout takes precedence on the fail one, and here at least one of
 	 * them is active
-- 
2.43.5
Re: [PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout_timer
Posted by Paolo Abeni 1 week, 1 day ago 
On 11/7/24 11:36, Dmitry Kandybka wrote:
> In 'mptcp_reset_tout_timer', promote 'probe_timestamp' to unsigned long
> to avoid possible integer overflow. Compile tested only.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Kandybka <d.kandybka@gmail.com>
> ---
>  net/mptcp/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e978e05ec8d1..ff2b8a2bfe18 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2722,8 +2722,8 @@ void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout)
>  	if (!fail_tout && !inet_csk(sk)->icsk_mtup.probe_timestamp)
>  		return;
>  
> -	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies +
> -			mptcp_close_timeout(sk);
> +	close_timeout = (unsigned long)inet_csk(sk)->icsk_mtup.probe_timestamp -
> +			tcp_jiffies32 + jiffies + mptcp_close_timeout(sk);
>  
>  	/* the close timeout takes precedence on the fail one, and here at least one of
>  	 * them is active

The patch makes sense to me. Any functional effect is hard to observe as
the timeout is served by the mptcp_worker, that can and is triggered
also by other events and uses the correct expression to evaluate the
timeout occurred event.

@Mat: are you ok with the patch?

Thanks,

Paolo
Re: [PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout_timer
Posted by Matthieu Baerts 1 week, 5 days ago 
Hi Dmitry,

On 07/11/2024 11:36, Dmitry Kandybka wrote:
> In 'mptcp_reset_tout_timer', promote 'probe_timestamp' to unsigned long
> to avoid possible integer overflow. Compile tested only.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Kandybka <d.kandybka@gmail.com>
> ---
>  net/mptcp/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e978e05ec8d1..ff2b8a2bfe18 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2722,8 +2722,8 @@ void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout)
>  	if (!fail_tout && !inet_csk(sk)->icsk_mtup.probe_timestamp)
>  		return;
>  
> -	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies +
> -			mptcp_close_timeout(sk);
> +	close_timeout = (unsigned long)inet_csk(sk)->icsk_mtup.probe_timestamp -
> +			tcp_jiffies32 + jiffies + mptcp_close_timeout(sk);

If I'm not mistaken, "jiffies" is an "unsigned long", which makes this
modification not necessary, no?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout_timer
Posted by Fedor Pchelkin 1 week, 5 days ago 
Hi,

Cc'ing more people.

On Fri, 08. Nov 12:43, Matthieu Baerts wrote:
> Hi Dmitry,
> 
> On 07/11/2024 11:36, Dmitry Kandybka wrote:
> > In 'mptcp_reset_tout_timer', promote 'probe_timestamp' to unsigned long
> > to avoid possible integer overflow. Compile tested only.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > 
> > Signed-off-by: Dmitry Kandybka <d.kandybka@gmail.com>
> > ---
> >  net/mptcp/protocol.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index e978e05ec8d1..ff2b8a2bfe18 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2722,8 +2722,8 @@ void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout)
> >  	if (!fail_tout && !inet_csk(sk)->icsk_mtup.probe_timestamp)
> >  		return;
> >  
> > -	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies +
> > -			mptcp_close_timeout(sk);
> > +	close_timeout = (unsigned long)inet_csk(sk)->icsk_mtup.probe_timestamp -
> > +			tcp_jiffies32 + jiffies + mptcp_close_timeout(sk);
> 
> If I'm not mistaken, "jiffies" is an "unsigned long", which makes this
> modification not necessary, no?

inet_csk(sk)->icsk_mtup.probe_timestamp and tcp_jiffies32 are both of u32
type.

'inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32' will be computed
first in u32, only then the result will be converted to unsigned long for
further calculations with 'jiffies'.

Looking at how probe_timestamp is initialized, seems it will always be less
than the current tcp_jiffies32 value.

So 'inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32' will wrap in
u32, and then converted to unsigned long. It's not clear actually whether
this is considered to be an expected behavior... Goes all the way down to
76a13b315709 ("mptcp: invoke MP_FAIL response when needed").

> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
Re: [PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout_timer
Posted by MPTCP CI 1 week, 6 days ago 
Hi Dmitry,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11721572197

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/92d0607638c3
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=907289


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.