* Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer
2009-11-12 16:29 [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
@ 2009-11-12 16:49 ` Andrei Pelinescu-Onciul
2009-11-12 17:05 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Andrei Pelinescu-Onciul @ 2009-11-12 16:49 UTC (permalink / raw)
To: linux-sctp
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when
2009-11-12 16:29 [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
2009-11-12 16:49 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer Andrei Pelinescu-Onciul
@ 2009-11-12 17:05 ` Vlad Yasevich
2009-11-12 17:16 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer Andrei Pelinescu-Onciul
2009-11-12 17:34 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
3 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2009-11-12 17:05 UTC (permalink / raw)
To: linux-sctp
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer
2009-11-12 16:29 [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
2009-11-12 16:49 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer Andrei Pelinescu-Onciul
2009-11-12 17:05 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
@ 2009-11-12 17:16 ` Andrei Pelinescu-Onciul
2009-11-12 17:34 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
3 siblings, 0 replies; 5+ messages in thread
From: Andrei Pelinescu-Onciul @ 2009-11-12 17:16 UTC (permalink / raw)
To: linux-sctp
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when
2009-11-12 16:29 [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when Vlad Yasevich
` (2 preceding siblings ...)
2009-11-12 17:16 ` [Lksctp-developers] [PATCH 2/3] sctp: fix integer overflow when setting the autoclose timer Andrei Pelinescu-Onciul
@ 2009-11-12 17:34 ` Vlad Yasevich
3 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2009-11-12 17:34 UTC (permalink / raw)
To: linux-sctp
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread