sctp: fix incorrect overflow check on autoclose
Subsystems (3)
PATCH RESEND 1/1 sctp: fix incorrect overflow check on autoclose
The commit 8ffd3208 voids the previous patches f6778aab and 810c0719 for limiting the maximum autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to 0xffffffff.
--- net/sctp/socket.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
net/sctp/socket.c +2 -1
@@ -2201,7 +2201,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,if (copy_from_user (&sp->autoclose, optval, optlen))return -EFAULT ;/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose =min_t (long, sp->autoclose,MAX_SCHEDULE_TIMEOUT /HZ ); + sp->autoclose = min_t(unsigned long, sp->autoclose, +MAX_SCHEDULE_TIMEOUT /HZ );return 0 ; }
Signature · 1 line
1.7.5.4
Quoted 19 lines · Xi Wang
The commit
8ffd3208voids the previous patchesf6778aaband810c0719for limiting the maximum autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to0xffffffff.Signed-off-by Xi Wang <[email protected]>Cc Andrew Morton <[email protected]>Cc Andrei Pelinescu-Onciul <[email protected]>Cc Vlad Yasevich <[email protected]>Cc David S. Miller <[email protected]>--- net/sctp/socket.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)net/sctp/socket.c +0
-0
net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2205
Quoted 6 lines
net/sctp/socket.c +2
-1@@ -2201,7 +2201,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,if (copy_from_user (&sp->autoclose, optval, optlen))return -EFAULT ;/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose =min_t (long, sp->autoclose,MAX_SCHEDULE_TIMEOUT /HZ ); + sp->autoclose = min_t(unsigned long, sp->autoclose, +MAX_SCHEDULE_TIMEOUT /HZ );
I think this should be u32 since that's what sp->autoclose is.
-vlad
Quoted 3 lines
net/sctp/socket.c +0
-0return 0 ; }
Quoted 1 line · Vladislav Yasevich
I think this should be u32 since that's what
sp->autocloseis.
Do you mean something like this?
min_t(u32, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ)
On 64-bit platform this would limit autoclose for no good reason to
(u32)(MAX_SCHEDULE_TIMEOUT / HZ),
which is basically 0x978d4fdf (assuming HZ is 250). I guess the intention was to allow autoclose to be any u32 on 64-bit platform.
- xi
Quoted 7 lines · Xi Wang
Quoted 1 line · Vladislav Yasevich
I think this should be u32 since that's what
sp->autocloseis.Do you mean something like this?
min_t(u32, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ)
Quoted 8 lines
On 64-bit platform this would limit autoclose for no good reason to
(u32)(MAX_SCHEDULE_TIMEOUT / HZ),which is basically
0x978d4fdf(assuming HZ is 250). I guess the intention was to allow autoclose to be any u32 on 64-bit platform.
Hm.. this is a bit strange. This makes it so that on 32 bit platforms we have one upper bound for autoclose and on 64 we have another even though the type is platform dependent. This could be considered a regression by applications.
In addition this would result in confusion to user since the values between setsockopt() and getsockopt() for autoclose would be different.
Looking at the latest spec, it actually looks like we are completely mis-interpreting autoclose. It's supposed to be in seconds.
For now, I'd suggest to make this consistent between 32 and 64 bits. Having inconsistent values seems strange.
As a next set the api needs to be fixed to accept seconds as argument.
-vlad
Quoted 4 lines · Vladislav Yasevich
Hm.. this is a bit strange. This makes it so that on 32 bit platforms we have one upper bound for autoclose and on 64 we have another even though the type is platform dependent. This could be considered a regression by applications.
Either looks good to me. Timeout limit is essentially different on 32/64 platforms.
Another (probably uglier) option is to limit the value on 32-bit platform only, like sock_setsockopt() in net/core/sock.c.
#if (BITS_PER_LONG = 32) if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ) sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ;
#endif
Quoted 2 lines
In addition this would result in confusion to user since the values between
setsockopt()andgetsockopt()for autoclose would be different.
Are you suggesting to reject the value and return -EINVAL, rather than silently limiting the autoclose value?
- xi
Quoted 8 lines · Xi Wang
Quoted 4 lines · Vladislav Yasevich
Hm.. this is a bit strange. This makes it so that on 32 bit platforms we have one upper bound for autoclose and on 64 we have another even though the type is platform dependent. This could be considered a regression by applications.
Either looks good to me. Timeout limit is essentially different on 32/64 platforms.
I don't think it really should be different. Notice that our rto values remain consistent. I really thing that this should be consistent from the user's point of view.
Quoted 8 lines
Another (probably uglier) option is to limit the value on 32-bit platform only, like
sock_setsockopt()innet/core/sock.c.#if (BITS_PER_LONG = 32) if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ) sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ;#endif
I agree, this is ugly. It might make more sense to define a max autoclose value and expose it through /sys. That way the values remains consistent.
-vlad
Quoted 9 lines
Quoted 3 lines
In addition this would result in confusion to user since the values between
setsockopt()andgetsockopt()for autoclose would be different.Are you suggesting to reject the value and return
-EINVAL, rather than silently limiting the autoclose value?- xi
Quoted 2 lines · Vladislav Yasevich
I agree, this is ugly. It might make more sense to define a max autoclose value and expose it through /sys. That way the values remains consistent.
Hmm, this sounds nice. I will give it a try in v2.
- xi
PATCH v2 1/1 sctp: fix incorrect overflow check on autoclose
In reply to Xi WangCommit 8ffd3208 voids the previous patches f6778aab and 810c0719 for limiting the maximum autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to 0xffffffff.
This patch defines a max_autoclose for limiting the value and exposes it through sysctl.
--- include/net/sctp/structs.h | 4 ++++ net/sctp/protocol.c | 3 +++ net/sctp/socket.c | 4 ++-- net/sctp/sysctl.c | 7 +++++++ 4 files changed, 16 insertions(+), 2 deletions(-)
include/net/sctp/structs.h +4 -0
@@ -241,6 +241,9 @@ extern struct sctp_globals {* bits is an indicator of when to send and window update SACK. */ int rwnd_update_shift; + +/* Threshold for autoclose timeout. */ +unsigned int max_autoclose; } sctp_globals;#define sctp_rto_initial (sctp_globals.rto_initial)
@@ -281,6 +284,7 @@ extern struct sctp_globals {#define sctp_auth_enable (sctp_globals.auth_enable)#define sctp_checksum_disable (sctp_globals.checksum_disable)#define sctp_rwnd_upd_shift (sctp_globals.rwnd_update_shift) +#define sctp_max_autoclose (sctp_globals.max_autoclose)/* SCTP Socket type: UDP or TCP style. */ typedef enum {
net/sctp/protocol.c +3 -0
@@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void) sctp_max_instreams =SCTP_DEFAULT_INSTREAMS ; sctp_max_outstreams =SCTP_DEFAULT_OUTSTREAMS ; +/* Initialize maximum autoclose timeout. */ + sctp_max_autoclose =INT_MAX ; +/* Initialize handle used for association ids. */ idr_init (&sctp_assocs_id);
net/sctp/socket.c +2 -2
@@ -2200,8 +2200,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,return -EINVAL ;if (copy_from_user (&sp->autoclose, optval, optlen))return -EFAULT ; -/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose =min_t (long, sp->autoclose,MAX_SCHEDULE_TIMEOUT /HZ ); +/* make sure it won't exceed sctp_max_autoclose / HZ */ + sp->autoclose =min (sp->autoclose, sctp_max_autoclose /HZ );return 0 ; }
net/sctp/sysctl.c +7 -0
@@ -258,6 +258,13 @@ static ctl_table sctp_table[] = { .extra1 = &one, .extra2 = &rwnd_scale_max, }, + { + .procname = "max_autoclose", + .data = &sctp_max_autoclose, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = &proc_dointvec_jiffies, + }, {/* sentinel */ } };
Signature · 1 line
1.7.5.4
Quoted 74 lines · Xi Wang
Commit
8ffd3208voids the previous patchesf6778aaband810c0719for limiting the maximum autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to0xffffffff.This patch defines a
max_autoclosefor limiting the value and exposes it through sysctl.Suggested-by Vlad Yasevich <[email protected]>Signed-off-by Xi Wang <[email protected]>--- include/net/sctp/structs.h | 4 ++++ net/sctp/protocol.c | 3 +++ net/sctp/socket.c | 4 ++-- net/sctp/sysctl.c | 7 +++++++ 4 files changed, 16 insertions(+), 2 deletions(-)include/net/sctp/structs.h +4
-0@@ -241,6 +241,9 @@ extern struct sctp_globals {* bits is an indicator of when to send and window update SACK. */ int rwnd_update_shift; + +/* Threshold for autoclose timeout. */ +unsigned int max_autoclose; } sctp_globals;#define sctp_rto_initial (sctp_globals.rto_initial)@@ -281,6 +284,7 @@ extern struct sctp_globals {#define sctp_auth_enable (sctp_globals.auth_enable)#define sctp_checksum_disable (sctp_globals.checksum_disable)#define sctp_rwnd_upd_shift (sctp_globals.rwnd_update_shift) +#define sctp_max_autoclose (sctp_globals.max_autoclose)/* SCTP Socket type: UDP or TCP style. */ typedef enum {net/sctp/protocol.c +3
-0@@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void) sctp_max_instreams =SCTP_DEFAULT_INSTREAMS ; sctp_max_outstreams =SCTP_DEFAULT_OUTSTREAMS ; +/* Initialize maximum autoclose timeout. */ + sctp_max_autoclose =INT_MAX ; +/* Initialize handle used for association ids. */ idr_init (&sctp_assocs_id);net/sctp/socket.c +2
-2@@ -2200,8 +2200,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,return -EINVAL ;if (copy_from_user (&sp->autoclose, optval, optlen))return -EFAULT ; -/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose =min_t (long, sp->autoclose,MAX_SCHEDULE_TIMEOUT /HZ ); +/* make sure it won't exceed sctp_max_autoclose / HZ */ + sp->autoclose =min (sp->autoclose, sctp_max_autoclose /HZ );return 0 ; }net/sctp/sysctl.c +0
-0@@ -258,6 +258,13 @@ static ctl_table sctp_table[] = { .extra1 = &one,
net/sctp/sysctl.c static ctl_table sctp_table[] = { · line 268
Quoted 10 lines
net/sctp/sysctl.c +7
-0.extra2 = &rwnd_scale_max, }, + { + .procname = "max_autoclose", + .data = &sctp_max_autoclose, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = &proc_dointvec_jiffies, + },
I think it would be better to keep this value in seconds and get rid of division in the setsockopt code. We could then have a min and max values where max value could be something like 2 days. I really don't see an autoclose value that is bigger then that being very useful. In fact, most of the time these values are very small as one wants to close out idle associations.
-vlad
Quoted 2 lines
net/sctp/sysctl.c +0
-0{/* sentinel */ } };
Quoted 6 lines · Vlad Yasevich
I think it would be better to keep this value in seconds and get rid of division in the setsockopt code. We could then have a min and max values where max value could be something like 2 days. I really don't see an autoclose value that is bigger then that being very useful. In fact, most of the time these values are very small as one wants to close out idle associations.
Now I start to think exposing a new sysctl option might be a little overkill since autoclose is often small.
How about this?
1) Simply store autoclose in seconds in setsockopt.
2) Avoid overflow in associola.c.
asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ) ? MAX_SCHEDULE_TIMEOUT : (unsigned long)sp->autoclose * HZ;
Or we could use INT_MAX instead of MAX_SCHEDULE_TIMEOUT if you want to keep that value consistent across 32/64 bits.
- xi
Quoted 15 lines · Xi Wang
Quoted 6 lines · Vlad Yasevich
I think it would be better to keep this value in seconds and get rid of division in the setsockopt code. We could then have a min and max values where max value could be something like 2 days. I really don't see an autoclose value that is bigger then that being very useful. In fact, most of the time these values are very small as one wants to close out idle associations.
Now I start to think exposing a new sysctl option might be a little overkill since autoclose is often small.
How about this?
1) Simply store autoclose in seconds in setsockopt.
Quoted 8 lines
2) Avoid overflow in
associola.c.asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ) ? MAX_SCHEDULE_TIMEOUT : (unsigned long)sp->autoclose * HZ;Or we could use
INT_MAXinstead ofMAX_SCHEDULE_TIMEOUTif you want to keep that value consistent across 32/64 bits.
This would work as well. I do like the max configurable though as it might be a nice feature, but the above code is exactly what I was thinking about too.
-vlad
Quoted 3 lines
- xi
Quoted 3 lines · Vlad Yasevich
This would work as well. I do like the max configurable though as it might be a nice feature, but the above code is exactly what I was thinking about too.
Okay I will do a v3 based on that with the max autoclose. ;-)
- xi
PATCH v3 1/1 sctp: fix incorrect overflow check on autoclose
In reply to Xi WangCommit 8ffd3208 voids the previous patches f6778aab and 810c0719 for limiting the autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to 0xffffffff.
This patch defines a max_autoclose (in seconds) for limiting the value and exposes it through sysctl, with the following intentions.
1) Avoid overflowing autoclose * HZ.
2) Keep the default autoclose bound consistent across 32- and 64-bit platforms (INT_MAX / HZ in this patch).
3) Keep the autoclose value consistent between setsockopt() and getsockopt() calls.
--- include/net/sctp/structs.h | 4 ++++ net/sctp/associola.c | 2 +- net/sctp/protocol.c | 3 +++ net/sctp/socket.c | 2 -- net/sctp/sysctl.c | 13 +++++++++++++ 5 files changed, 21 insertions(+), 3 deletions(-)
include/net/sctp/structs.h +4 -0
@@ -241,6 +241,9 @@ extern struct sctp_globals {* bits is an indicator of when to send and window update SACK. */ int rwnd_update_shift; + +/* Threshold for autoclose timeout, in seconds. */ +unsigned long max_autoclose; } sctp_globals;#define sctp_rto_initial (sctp_globals.rto_initial)
@@ -281,6 +284,7 @@ extern struct sctp_globals {#define sctp_auth_enable (sctp_globals.auth_enable)#define sctp_checksum_disable (sctp_globals.checksum_disable)#define sctp_rwnd_upd_shift (sctp_globals.rwnd_update_shift) +#define sctp_max_autoclose (sctp_globals.max_autoclose)/* SCTP Socket type: UDP or TCP style. */ typedef enum {
net/sctp/associola.c +1 -0
@@ -173,7 +173,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 ] - (unsigned long )sp->autoclose *HZ ; + min_t(unsigned long, sp->autoclose, sctp_max_autoclose) * HZ;/* Initializes the timers */ for (i =SCTP_EVENT_TIMEOUT_NONE ; i <SCTP_NUM_TIMEOUT_TYPES ; ++i)
net/sctp/protocol.c +3 -0
@@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void) sctp_max_instreams =SCTP_DEFAULT_INSTREAMS ; sctp_max_outstreams =SCTP_DEFAULT_OUTSTREAMS ; +/* Initialize maximum autoclose timeout. */ + sctp_max_autoclose =INT_MAX /HZ ; +/* Initialize handle used for association ids. */ idr_init (&sctp_assocs_id);
net/sctp/socket.c +0 -2
@@ -2200,8 +2200,6 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,return -EINVAL ;if (copy_from_user (&sp->autoclose, optval, optlen))return -EFAULT ; -/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose =min_t (long, sp->autoclose,MAX_SCHEDULE_TIMEOUT /HZ );return 0 ; }
net/sctp/sysctl.c +12 -0
@@ -53,6 +53,10 @@ static int sack_timer_min = 1;static int sack_timer_max =500 ;static int addr_scope_max =3 ;/* check sctp_scope_policy_t in include/net/sctp/constants.h for max entries */ static int rwnd_scale_max =16 ; +static unsigned long max_autoclose_min =0 ; +static unsigned long max_autoclose_max + (MAX_SCHEDULE_TIMEOUT / HZ > UINT_MAX) + ?UINT_MAX :MAX_SCHEDULE_TIMEOUT /HZ ;extern long sysctl_sctp_mem[3 ];extern int sysctl_sctp_rmem[3 ];
@@ -258,6 +262,15 @@ static ctl_table sctp_table[] = { .extra1 = &one, .extra2 = &rwnd_scale_max, }, + { + .procname = "max_autoclose", + .data = &sctp_max_autoclose, + .maxlen = sizeof(unsigned long), + .mode = 0644, + .proc_handler = &proc_doulongvec_minmax, + .extra1 = &max_autoclose_min, + .extra2 = &max_autoclose_max, + }, {/* sentinel */ } };
Signature · 1 line
1.7.5.4
Quoted 10 lines · Xi Wang
Commit
8ffd3208voids the previous patchesf6778aaband810c0719for limiting the autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to0xffffffff.This patch defines a
max_autoclose(in seconds) for limiting the value and exposes it through sysctl, with the following intentions.1) Avoid overflowing autoclose * HZ.
Quoted 8 lines
2) Keep the default autoclose bound consistent across 32- and 64-bit platforms (
INT_MAX/ HZ in this patch).3) Keep the autoclose value consistent between
setsockopt()andgetsockopt()calls.Suggested-by Vlad Yasevich <[email protected]>Signed-off-by Xi Wang <[email protected]>
Looks good to me.
-vlad
P.S. Sorry, took so long. Just got back from a long vacation.
Quoted 99 lines
--- include/net/sctp/structs.h | 4 ++++ net/sctp/associola.c | 2 +- net/sctp/protocol.c | 3 +++ net/sctp/socket.c | 2 -- net/sctp/sysctl.c | 13 +++++++++++++ 5 files changed, 21 insertions(+), 3 deletions(-)include/net/sctp/structs.h +4
-0@@ -241,6 +241,9 @@ extern struct sctp_globals {* bits is an indicator of when to send and window update SACK. */ int rwnd_update_shift; + +/* Threshold for autoclose timeout, in seconds. */ +unsigned long max_autoclose; } sctp_globals;#define sctp_rto_initial (sctp_globals.rto_initial)@@ -281,6 +284,7 @@ extern struct sctp_globals {#define sctp_auth_enable (sctp_globals.auth_enable)#define sctp_checksum_disable (sctp_globals.checksum_disable)#define sctp_rwnd_upd_shift (sctp_globals.rwnd_update_shift) +#define sctp_max_autoclose (sctp_globals.max_autoclose)/* SCTP Socket type: UDP or TCP style. */ typedef enum {net/sctp/associola.c +1
-0@@ -173,7 +173,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 ] > - (unsigned long )sp->autoclose *HZ ; + min_t(unsigned long, sp->autoclose, sctp_max_autoclose) * HZ;/* Initializes the timers */ for (i =SCTP_EVENT_TIMEOUT_NONE ; i <SCTP_NUM_TIMEOUT_TYPES ; ++i)net/sctp/protocol.c +3
-0@@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void) sctp_max_instreams =SCTP_DEFAULT_INSTREAMS ; sctp_max_outstreams =SCTP_DEFAULT_OUTSTREAMS ; +/* Initialize maximum autoclose timeout. */ + sctp_max_autoclose =INT_MAX /HZ ; +/* Initialize handle used for association ids. */ idr_init (&sctp_assocs_id);net/sctp/socket.c +0
-2@@ -2200,8 +2200,6 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,return -EINVAL ;if (copy_from_user (&sp->autoclose, optval, optlen))return -EFAULT ; -/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose =min_t (long, sp->autoclose,MAX_SCHEDULE_TIMEOUT /HZ );return 0 ; }net/sctp/sysctl.c +12
-0@@ -53,6 +53,10 @@ static int sack_timer_min = 1;static int sack_timer_max =500 ;static int addr_scope_max =3 ;/* check sctp_scope_policy_t in include/net/sctp/constants.h for max entries */ static int rwnd_scale_max =16 ; +static unsigned long max_autoclose_min =0 ; +static unsigned long max_autoclose_max > + (MAX_SCHEDULE_TIMEOUT / HZ > UINT_MAX) + ?UINT_MAX :MAX_SCHEDULE_TIMEOUT /HZ ;extern long sysctl_sctp_mem[3 ];extern int sysctl_sctp_rmem[3 ];@@ -258,6 +262,15 @@ static ctl_table sctp_table[] = { .extra1 = &one, .extra2 = &rwnd_scale_max, }, + { + .procname = "max_autoclose", + .data = &sctp_max_autoclose, + .maxlen = sizeof(unsigned long), + .mode = 0644, + .proc_handler = &proc_doulongvec_minmax, + .extra1 = &max_autoclose_min, + .extra2 = &max_autoclose_max, + }, {/* sentinel */ } };