netdev - Re: [PATCH] mptcp: fix possible integer overflow in mptcp_reset_tout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241109-46edd978694b7d9a2f89b9f6-pchelkin@ispras.ru>
Date: Sat, 9 Nov 2024 00:42:53 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: Dmitry Kandybka <d.kandybka@...il.com>, netdev@...r.kernel.org,
	Dmitry Antipov <dmantipov@...dex.ru>, mptcp@...ts.linux.dev,
	lvc-project@...uxtesting.org, Mat Martineau <martineau@...nel.org>,
	Geliang Tang <geliang@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>
Subject: Re: [PATCH] mptcp: fix possible integer overflow in
 mptcp_reset_tout_timer

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@...il.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.