[PATCH v4] sctp: check the rto_min and rto_max

[PATCH v4] sctp: check the rto_min and rto_max

[Wang Weidong]

rto_min should be smaller than rto_max while rto_max should be larger
than rto_min. Add two proc_handler for the checking. Add the check in
sctp_setsockopt_rtoinfo.
delete a blank line in proc_sctp_do_hmac_alg() declaration.

Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/socket.c |  5 ++++
 net/sctp/sysctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 72046b9..2e1af1b 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
 	if (copy_from_user(&rtoinfo, optval, optlen))
 		return -EFAULT;
 
+	if (rtoinfo.srto_min < 1 ||
+	    rtoinfo.srto_max > 86400000 ||
+	    rtoinfo.srto_max < rtoinfo.srto_min)
+		return -EINVAL;
+
 	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
 
 	/* Set the values to the specific association */
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 6b36561..8d6757a 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -59,8 +59,16 @@ extern int sysctl_sctp_wmem[3];
 static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 				int write,
 				void __user *buffer, size_t *lenp,
-
 				loff_t *ppos);
+static int proc_sctp_do_rto_min(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+static int proc_sctp_do_rto_max(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+
 static struct ctl_table sctp_table[] = {
 	{
 		.procname	= "sctp_mem",
@@ -102,17 +110,17 @@ static struct ctl_table sctp_net_table[] = {
 		.data		= &init_net.sctp.rto_min,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= proc_sctp_do_rto_min,
 		.extra1         = &one,
-		.extra2         = &timer_max
+		.extra2         = &init_net.sctp.rto_max
 	},
 	{
 		.procname	= "rto_max",
 		.data		= &init_net.sctp.rto_max,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
+		.proc_handler	= proc_sctp_do_rto_max,
+		.extra1         = &init_net.sctp.rto_min,
 		.extra2         = &timer_max
 	},
 	{
@@ -342,6 +350,62 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 	return ret;
 }
 
+static int proc_sctp_do_rto_min(struct ctl_table *ctl,
+				int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	struct net *net = current->nsproxy->net_ns;
+	int new_value;
+	struct ctl_table tbl;
+	unsigned int min = *(unsigned int *) ctl->extra1;
+	unsigned int max = *(unsigned int *) ctl->extra2;
+	int ret;
+
+	memset(&tbl, 0, sizeof(struct ctl_table));
+	tbl.maxlen = sizeof(unsigned int);
+
+	if (write)
+		tbl.data = &new_value;
+	else
+		tbl.data = &net->sctp.rto_min;
+	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
+	if (write) {
+		if (ret || new_value > max || new_value < min)
+			return -EINVAL;
+		net->sctp.rto_min = new_value;
+	}
+	return ret;
+}
+
+static int proc_sctp_do_rto_max(struct ctl_table *ctl,
+				int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	struct net *net = current->nsproxy->net_ns;
+	int new_value;
+	struct ctl_table tbl;
+	unsigned int min = *(unsigned int *) ctl->extra1;
+	unsigned int max = *(unsigned int *) ctl->extra2;
+	int ret;
+
+	memset(&tbl, 0, sizeof(struct ctl_table));
+	tbl.maxlen = sizeof(unsigned int);
+
+	if (write)
+		tbl.data = &new_value;
+	else
+		tbl.data = &net->sctp.rto_max;
+	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
+	if (write) {
+                if (ret || new_value > max || new_value < min)
+			return -EINVAL;
+		net->sctp.rto_max = new_value;
+	}
+	return ret;
+}
+
 int sctp_sysctl_net_register(struct net *net)
 {
 	struct ctl_table *table;
-- 
1.7.12

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Neil Horman]

On Thu, Dec 05, 2013 at 10:19:25AM +0800, Wang Weidong wrote:
> rto_min should be smaller than rto_max while rto_max should be larger
> than rto_min. Add two proc_handler for the checking. Add the check in
> sctp_setsockopt_rtoinfo.
> delete a blank line in proc_sctp_do_hmac_alg() declaration.
> 
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>  net/sctp/socket.c |  5 ++++
>  net/sctp/sysctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 72046b9..2e1af1b 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>  	if (copy_from_user(&rtoinfo, optval, optlen))
>  		return -EFAULT;
>  
> +	if (rtoinfo.srto_min < 1 ||
> +	    rtoinfo.srto_max > 86400000 ||
These should be defiend to some descriptive value.

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Wang Weidong]

From: Wang Weidong <wangweidong1@huawei.com>

On 2013/12/5 21:32, Neil Horman wrote:
> On Thu, Dec 05, 2013 at 10:19:25AM +0800, Wang Weidong wrote:
>> rto_min should be smaller than rto_max while rto_max should be larger
>> than rto_min. Add two proc_handler for the checking. Add the check in
>> sctp_setsockopt_rtoinfo.
>> delete a blank line in proc_sctp_do_hmac_alg() declaration.
>>
>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>   net/sctp/socket.c |  5 ++++
>>   net/sctp/sysctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 74 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 72046b9..2e1af1b 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>   	if (copy_from_user(&rtoinfo, optval, optlen))
>>   		return -EFAULT;
>>
>> +	if (rtoinfo.srto_min < 1 ||
>> +	    rtoinfo.srto_max > 86400000 ||
> These should be defiend to some descriptive value.
>
Thanks for your suggestion. I will fix it in v5.

Regards.

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Daniel Borkmann]

On 12/05/2013 03:07 PM, Wang Weidong wrote:
> From: Wang Weidong <wangweidong1@huawei.com>
> On 2013/12/5 21:32, Neil Horman wrote:
>> On Thu, Dec 05, 2013 at 10:19:25AM +0800, Wang Weidong wrote:
>>> rto_min should be smaller than rto_max while rto_max should be larger
>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>> sctp_setsockopt_rtoinfo.
>>> delete a blank line in proc_sctp_do_hmac_alg() declaration.
>>>
>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>> ---
>>>   net/sctp/socket.c |  5 ++++
>>>   net/sctp/sysctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>   2 files changed, 74 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index 72046b9..2e1af1b 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>       if (copy_from_user(&rtoinfo, optval, optlen))
>>>           return -EFAULT;
>>>
>>> +    if (rtoinfo.srto_min < 1 ||
>>> +        rtoinfo.srto_max > 86400000 ||
>> These should be defiend to some descriptive value.
>>
> Thanks for your suggestion. I will fix it in v5.

While you're on v5 anyway, please also fix up spacing in your
commit from ...

static int proc_sctp_do_rto_min(struct ctl_table *ctl,
				int write,
				void __user*buffer, size_t *lenp,
				loff_t *ppos)

... to something like ...

static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
				void __user *buffer, size_t *lenp,
				loff_t *ppos)

Thanks !

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/6 0:25, Daniel Borkmann wrote:
> On 12/05/2013 03:07 PM, Wang Weidong wrote:
>> From: Wang Weidong <wangweidong1@huawei.com>
>> On 2013/12/5 21:32, Neil Horman wrote:
>>> On Thu, Dec 05, 2013 at 10:19:25AM +0800, Wang Weidong wrote:
>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>> sctp_setsockopt_rtoinfo.
>>>> delete a blank line in proc_sctp_do_hmac_alg() declaration.
>>>>
>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>> ---
>>>>   net/sctp/socket.c |  5 ++++
>>>>   net/sctp/sysctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>   2 files changed, 74 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>> index 72046b9..2e1af1b 100644
>>>> --- a/net/sctp/socket.c
>>>> +++ b/net/sctp/socket.c
>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>       if (copy_from_user(&rtoinfo, optval, optlen))
>>>>           return -EFAULT;
>>>>
>>>> +    if (rtoinfo.srto_min < 1 ||
>>>> +        rtoinfo.srto_max > 86400000 ||
>>> These should be defiend to some descriptive value.
>>>
>> Thanks for your suggestion. I will fix it in v5.
> 
> While you're on v5 anyway, please also fix up spacing in your
> commit from ...
> 
> static int proc_sctp_do_rto_min(struct ctl_table *ctl,
>                 int write,
>                 void __user*buffer, size_t *lenp,
>                 loff_t *ppos)
> 
> ... to something like ...
> 
> static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>                 void __user *buffer, size_t *lenp,
>                 loff_t *ppos)
> 
> Thanks !

Hm, I do it for according to the proc_sctp_do_hmac_alg which is in the same file.
If I fix this, I think it is better to fix the proc_sctp_do_hmac_alg as well. 
Should I do it a patch or another patch?

Thanks!

> -- 
> 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
> 
> 

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/5 21:32, Neil Horman wrote:
> On Thu, Dec 05, 2013 at 10:19:25AM +0800, Wang Weidong wrote:
>> rto_min should be smaller than rto_max while rto_max should be larger
>> than rto_min. Add two proc_handler for the checking. Add the check in
>> sctp_setsockopt_rtoinfo.
>> delete a blank line in proc_sctp_do_hmac_alg() declaration.
>>
>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>  net/sctp/socket.c |  5 ++++
>>  net/sctp/sysctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 74 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 72046b9..2e1af1b 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>  		return -EFAULT;
>>  
>> +	if (rtoinfo.srto_min < 1 ||
>> +	    rtoinfo.srto_max > 86400000 ||
> These should be defiend to some descriptive value.
> 
> 
Hi Neil,

Should I define it in net/sctp/sctp.h, so sysctl.c can use it as well.
Or I just define it in the socket.c same as the definition in sysctl.c?

Regards.
Wang

> 

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Daniel Borkmann]

On 12/06/2013 02:23 AM, Wang Weidong wrote:

> Hm, I do it for according to the proc_sctp_do_hmac_alg which is in the same file.
> If I fix this, I think it is better to fix the proc_sctp_do_hmac_alg as well.
> Should I do it a patch or another patch?

Sure, feel free.

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Daniel Borkmann]

On 12/06/2013 09:48 AM, Wang Weidong wrote:

> Should I define it in net/sctp/sctp.h, so sysctl.c can use it as well.
> Or I just define it in the socket.c same as the definition in sysctl.c?

Preferably in include/net/sctp/constants.h where we already have most
other constants anyway.

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/6 17:50, Daniel Borkmann wrote:
> On 12/06/2013 09:48 AM, Wang Weidong wrote:
> 
>> Should I define it in net/sctp/sctp.h, so sysctl.c can use it as well.
>> Or I just define it in the socket.c same as the definition in sysctl.c?
> 
> Preferably in include/net/sctp/constants.h where we already have most
> other constants anyway.
> 

Nice, Thanks for your suggestion.

Regards.
Wang
> 

Re: [PATCH v4] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/6 17:50, Daniel Borkmann wrote:
> On 12/06/2013 09:48 AM, Wang Weidong wrote:
> 
>> Should I define it in net/sctp/sctp.h, so sysctl.c can use it as well.
>> Or I just define it in the socket.c same as the definition in sysctl.c?
> 
> Preferably in include/net/sctp/constants.h where we already have most
> other constants anyway.
> 
> 
Hi Daniel,

in sysctl.c it will use a point to the value while the socket.c use the value.
So if I define macros SCTP_ONE or SCTP_TIMER_MAX in include/net/sctp/constants.h
which only used by socket.c. Is it OK?

Regards.
Wang