| Message ID | 20241107103657.1560536-1-d.kandybka@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b169e76ebad22cbd055101ee5aa1a7bed0e66606 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mptcp: fix possible integer overflow in mptcp_reset_tout_timer | expand |
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
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.
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
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 7 Nov 2024 13:36:57 +0300 you 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> > > [...] Here is the summary with links: - mptcp: fix possible integer overflow in mptcp_reset_tout_timer https://git.kernel.org/netdev/net-next/c/b169e76ebad2 You are awesome, thank you!
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
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(-)