[PATCH] net/sctp: Prevent autoclose integer overflow in sctp_association_init()

[PATCH] net/sctp: Prevent autoclose integer overflow in sctp_association_init()

[Nikolay Kuratov]

While by default max_autoclose equals to INT_MAX / HZ, one may set
net.sctp.max_autoclose to UINT_MAX. There is code in
sctp_association_init() that can consequently trigger overflow.

Cc: stable@vger.kernel.org
Fixes: 9f70f46bd4c7 ("sctp: properly latch and use autoclose value from sock to association")
Signed-off-by: Nikolay Kuratov <kniv@yandex-team.ru>
---
 net/sctp/associola.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index c45c192b7878..0b0794f164cf 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -137,7 +137,8 @@ static struct sctp_association *sctp_association_init(
 		= 5 * asoc->rto_max;
 
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
+	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
+		(unsigned long)sp->autoclose * HZ;
 
 	/* Initializes the timers */
 	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
-- 
2.34.1

Re: [PATCH] net/sctp: Prevent autoclose integer overflow in sctp_association_init()

[Xin Long]

On Thu, Dec 19, 2024 at 11:21 AM Nikolay Kuratov <kniv@yandex-team.ru> wrote:
>
> While by default max_autoclose equals to INT_MAX / HZ, one may set
> net.sctp.max_autoclose to UINT_MAX. There is code in
> sctp_association_init() that can consequently trigger overflow.
>
> Cc: stable@vger.kernel.org
> Fixes: 9f70f46bd4c7 ("sctp: properly latch and use autoclose value from sock to association")
> Signed-off-by: Nikolay Kuratov <kniv@yandex-team.ru>
> ---
>  net/sctp/associola.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index c45c192b7878..0b0794f164cf 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -137,7 +137,8 @@ static struct sctp_association *sctp_association_init(
>                 = 5 * asoc->rto_max;
>
>         asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
> -       asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
> +       asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
> +               (unsigned long)sp->autoclose * HZ;
>
>         /* Initializes the timers */
>         for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
> --
> 2.34.1
>
Acked-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH] net/sctp: Prevent autoclose integer overflow in sctp_association_init()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 19 Dec 2024 19:21:14 +0300 you wrote:
> While by default max_autoclose equals to INT_MAX / HZ, one may set
> net.sctp.max_autoclose to UINT_MAX. There is code in
> sctp_association_init() that can consequently trigger overflow.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9f70f46bd4c7 ("sctp: properly latch and use autoclose value from sock to association")
> Signed-off-by: Nikolay Kuratov <kniv@yandex-team.ru>
> 
> [...]

Here is the summary with links:
  - net/sctp: Prevent autoclose integer overflow in sctp_association_init()
    https://git.kernel.org/netdev/net/c/4e86729d1ff3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net/sctp: Prevent autoclose integer overflow in sctp_association_init()

[David Laight]

On Thu, 19 Dec 2024 19:21:14 +0300
Nikolay Kuratov <kniv@yandex-team.ru> wrote:

> While by default max_autoclose equals to INT_MAX / HZ, one may set
> net.sctp.max_autoclose to UINT_MAX. There is code in
> sctp_association_init() that can consequently trigger overflow.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9f70f46bd4c7 ("sctp: properly latch and use autoclose value from sock to association")
> Signed-off-by: Nikolay Kuratov <kniv@yandex-team.ru>
> ---
>  net/sctp/associola.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index c45c192b7878..0b0794f164cf 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -137,7 +137,8 @@ static struct sctp_association *sctp_association_init(
>  		= 5 * asoc->rto_max;
>  
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
> -	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
> +	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
> +		(unsigned long)sp->autoclose * HZ;

That doesn't fix 32bit systems.

Looking through sctp/structs.h there are a lot of 'long' used for
timeouts.
it can't be right that any of these change size between 32bit and 64bit.
So they should either be __u32 or __u64 (or similar).

	David

>  
>  	/* Initializes the timers */
>  	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)

Re: [PATCH] net/sctp: Prevent autoclose integer overflow in sctp_association_init()

[Fedor Pchelkin]

David Laight wrote:
> That doesn't fix 32bit systems.
> 
> Looking through sctp/structs.h there are a lot of 'long' used for
> timeouts.
> it can't be right that any of these change size between 32bit and 64bit.
> So they should either be __u32 or __u64 (or similar).

I guess all the 'long' timeout values - not only in sctp, this concerns
other modules in general - follow the jiffies thing itself which is
declared as 'unsigned long'.