Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when

Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when

[Vlad Yasevich]

Andrei Pelinescu-Onciul wrote:
> When setting the autoclose timeout in jiffies there is a possible
> integer overflow if the value in seconds is very large
> (e.g. for 2^22 s with HZ\x1024). The problem appears even on
> 64-bit due to the integer promotion rules. The fix is just a cast
>  to unsigned long.
> 
> Signed-off-by: Andrei Pelinescu-Onciul <andrei@iptel.org>
> ---
>  net/sctp/associola.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 525864b..7f69f4d 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -166,7 +166,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > -		sp->autoclose * HZ;
> +		(unsigned long)sp->autoclose * HZ;
>  
>  	/* Initilizes the timers */
>  	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)

This becomes unnecessary with Patch 3.


-vlad

Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer

[Andrei Pelinescu-Onciul]

On Nov 12, 2009 at 11:29, Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
> 
> 
> Andrei Pelinescu-Onciul wrote:
> > When setting the autoclose timeout in jiffies there is a possible
> > integer overflow if the value in seconds is very large
> > (e.g. for 2^22 s with HZ\x1024). The problem appears even on
> > 64-bit due to the integer promotion rules. The fix is just a cast
> >  to unsigned long.
> > 
> > Signed-off-by: Andrei Pelinescu-Onciul <andrei@iptel.org>
> > ---
> >  net/sctp/associola.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 525864b..7f69f4d 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -166,7 +166,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
> >  	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
> >  	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
> >  	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > > -		sp->autoclose * HZ;
> > +		(unsigned long)sp->autoclose * HZ;
> >  
> >  	/* Initilizes the timers */
> >  	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
> 
> This becomes unnecessary with Patch 3.

I don't think so. Patch 3 makes sure 
sp->autoclose <= (MAX_SCHEDULE_TIMEOUT / HZ).
On 64 bits this is always true, because autoclose is u32,
MAX_SCHEDULE_TIMEOUT is LONG_MAX (2^63-1) and HZ <= 1024, so on 64 bits
patch 3 will not do anything and sp->autoclose * HZ can still overflow
an int.
E.g.: autoclose= 2^22, HZ\x1024.
2^22 < (2^63-1)/1024 => patch 3 does not change autoclose
However  2^22 * 1024 = 2^32 which is > UINT_MAX =>
asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] will be set to
(uint)2^32 = 0!


Andrei

Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when

[Vlad Yasevich]

Andrei Pelinescu-Onciul wrote:
> On Nov 12, 2009 at 11:29, Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
>>
>> Andrei Pelinescu-Onciul wrote:
>>> When setting the autoclose timeout in jiffies there is a possible
>>> integer overflow if the value in seconds is very large
>>> (e.g. for 2^22 s with HZ\x1024). The problem appears even on
>>> 64-bit due to the integer promotion rules. The fix is just a cast
>>>  to unsigned long.
>>>
>>> Signed-off-by: Andrei Pelinescu-Onciul <andrei@iptel.org>
>>> ---
>>>  net/sctp/associola.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>>> index 525864b..7f69f4d 100644
>>> --- a/net/sctp/associola.c
>>> +++ b/net/sctp/associola.c
>>> @@ -166,7 +166,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>>>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
>>>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
>>>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] >>> -		sp->autoclose * HZ;
>>> +		(unsigned long)sp->autoclose * HZ;
>>>  
>>>  	/* Initilizes the timers */
>>>  	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
>> This becomes unnecessary with Patch 3.
> 
> I don't think so. Patch 3 makes sure 
> sp->autoclose <= (MAX_SCHEDULE_TIMEOUT / HZ).
> On 64 bits this is always true, because autoclose is u32,
> MAX_SCHEDULE_TIMEOUT is LONG_MAX (2^63-1) and HZ <= 1024, so on 64 bits
> patch 3 will not do anything and sp->autoclose * HZ can still overflow
> an int.
> E.g.: autoclose= 2^22, HZ\x1024.
> 2^22 < (2^63-1)/1024 => patch 3 does not change autoclose
> However  2^22 * 1024 = 2^32 which is > UINT_MAX =>
> asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] will be set to
> (uint)2^32 = 0!
> 

Ok.  So, we'll change patch 3 to do:

if (sp->autoclose * HZ > MAX_SCHEDULE_TIMEOUT)
	sp->autoclose = MAX_SCHEDULE_TIMEOUT/HZ;

-vlad


> 
> Andrei
> 
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
> trial. Simplify your report design, integration and deployment - and focus on 
> what you do best, core application coding. Discover what's new with
> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
> _______________________________________________
> Lksctp-developers mailing list
> Lksctp-developers@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lksctp-developers
> 

Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer

[Andrei Pelinescu-Onciul]

On Nov 12, 2009 at 12:05, Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
> 
> 
> Andrei Pelinescu-Onciul wrote:
> > On Nov 12, 2009 at 11:29, Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
> >>
> >> Andrei Pelinescu-Onciul wrote:
> >>> When setting the autoclose timeout in jiffies there is a possible
> >>> integer overflow if the value in seconds is very large
> >>> (e.g. for 2^22 s with HZ\x1024). The problem appears even on
> >>> 64-bit due to the integer promotion rules. The fix is just a cast
> >>>  to unsigned long.
> >>>
> >>> Signed-off-by: Andrei Pelinescu-Onciul <andrei@iptel.org>
> >>> ---
> >>>  net/sctp/associola.c |    2 +-
> >>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> >>> index 525864b..7f69f4d 100644
> >>> --- a/net/sctp/associola.c
> >>> +++ b/net/sctp/associola.c
> >>> @@ -166,7 +166,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
> >>>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
> >>>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
> >>>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > >>> -		sp->autoclose * HZ;
> >>> +		(unsigned long)sp->autoclose * HZ;
> >>>  
> >>>  	/* Initilizes the timers */
> >>>  	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
> >> This becomes unnecessary with Patch 3.
> > 
> > I don't think so. Patch 3 makes sure 
> > sp->autoclose <= (MAX_SCHEDULE_TIMEOUT / HZ).
> > On 64 bits this is always true, because autoclose is u32,
> > MAX_SCHEDULE_TIMEOUT is LONG_MAX (2^63-1) and HZ <= 1024, so on 64 bits
> > patch 3 will not do anything and sp->autoclose * HZ can still overflow
> > an int.
> > E.g.: autoclose= 2^22, HZ\x1024.
> > 2^22 < (2^63-1)/1024 => patch 3 does not change autoclose
> > However  2^22 * 1024 = 2^32 which is > UINT_MAX =>
> > asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] will be set to
> > (uint)2^32 = 0!
> > 
> 
> Ok.  So, we'll change patch 3 to do:
> 
> if (sp->autoclose * HZ > MAX_SCHEDULE_TIMEOUT)
> 	sp->autoclose = MAX_SCHEDULE_TIMEOUT/HZ;

It will still not work on 64 bits (always true) and it will also won't work
anymore on 32 bits (e.g. on 32 bits: 2^22 * 1024 > 2^31-1 it's false
 and henve autoclose won't be fixed).

If you want to do it from sctp_setsockopt_autoclose() you would have
 to add another  condition, something like:

/* 32 bits: */
if ((sp->autoclose > (MAX_SCHEDULE_TIMEOUT / HZ) )
...

/* 64 bits hack */
if (sp->autoclose > (UINT_MAX / HZ))
    sp->autoclose = UINT_MAX / HZ;

but in this case you'll reduce the maximum interval on 64 bits for no
good reason.

The  (unsigned long) cast in the patch above (sctp_association_init()),
solves the problem on 64 bits, has no performance impact, allows for
larger timeouts on 64 bits  and is more elegant (well at least from my
point of view).

Andrei

Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when

[Vlad Yasevich]

Andrei Pelinescu-Onciul wrote:
> 
> The  (unsigned long) cast in the patch above (sctp_association_init()),
> solves the problem on 64 bits, has no performance impact, allows for
> larger timeouts on 64 bits  and is more elegant (well at least from my
> point of view).

You've convinced me.  Queued.

-vlad

> 
> Andrei
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>