Patchwork [v2] sctp: fix incorrect overflow check on autoclose

login
register
mail settings
Submitter Xi Wang
Date Dec. 14, 2011, 9:48 p.m.
Message ID <4EE919B5.3090901@gmail.com>
Download mbox | patch
Permalink /patch/131481/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Xi Wang - Dec. 14, 2011, 9:48 p.m.
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.

This patch defines a max_autoclose for limiting the value and exposes
it through sysctl.

Suggested-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 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(-)
Vlad Yasevich - Dec. 15, 2011, 9:07 p.m.
On 12/14/2011 04:48 PM, Xi Wang wrote:
> 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.
> 
> This patch defines a max_autoclose for limiting the value and exposes
> it through sysctl.
> 
> Suggested-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  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(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index e90e7a9..781f16d 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -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 {
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 61b9fca..4e07b18 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -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);
>  
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 13bf5fc..f8c8e66 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -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;
>  }
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 6b39529..b74686e 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -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,
> +	},
>

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

>  	{ /* sentinel */ }
>  };
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang - Dec. 15, 2011, 10:13 p.m.
On Dec 15, 2011, at 4:07 PM, Vlad Yasevich wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich - Dec. 16, 2011, 1 p.m.
On 12/15/2011 05:13 PM, Xi Wang wrote:
> On Dec 15, 2011, at 4:07 PM, Vlad Yasevich wrote:
>> 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.

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

> 
> - xi
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang - Dec. 16, 2011, 10:25 p.m.
On Dec 16, 2011, at 8:00 AM, Vlad Yasevich wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index e90e7a9..781f16d 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -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 {
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 61b9fca..4e07b18 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -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);
 
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 13bf5fc..f8c8e66 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -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;
 }
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 6b39529..b74686e 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -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 */ }
 };