sctp: check the rto_min and rto_max

PATCH Superseded Wang Weidong 2 participants 8 messages Dec 2, 2013 – Dec 4, 2013

Subsystems (3)

Networking Network drivers SCTP protocol

lore

Share & tools

lore

Wang Weidong

author

2013-12-02 01:45 UTC #0 lore raw

PATCH v2 1/1 sctp: check the rto_min and rto_max

rto_min should be smaller than rto_max while rto_max should be larger than rto_min. so just add the check.

Acked-by Neil Horman <[email protected]>

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/sysctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/sctp/sysctl.c +2 -2

index 6b36561..7637e8e 100644
@@ -104,7 +104,7 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1         = &one,
-		.extra2         = &timer_max
+		.extra2         = &init_net.sctp.rto_max
 	},
 	{
 		.procname	= "rto_max",

@@ -112,7 +112,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
+		.extra1         = &init_net.sctp.rto_min,
 		.extra2         = &timer_max
 	},
 	{

Signature · 1 line

1.7.12

Wang Weidong

author

2013-12-02 12:15 UTC #1 lore raw

Quoted 24 lines · Wang Weidong

rto_min should be smaller than rto_max while rto_max should be larger than rto_min. so just add the check.
Acked-by Neil Horman <[email protected]>
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/sctp/sysctl.c +1 -1
index 6b36561..7637e8e 100644
@@ -104,7 +104,7 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1         = &one,
-		.extra2         = &timer_max
+		.extra2         = &init_net.sctp.rto_max
 	},
 	{
 		.procname	= "rto_max",

net/sctp/sysctl.c static struct ctl_table sctp_net_table[] = { · line 118

Quoted 8 lines

net/sctp/sysctl.c +1 -1

@@ -112,7 +112,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
+		.extra1         = &init_net.sctp.rto_min,
 		.extra2         = &timer_max
 	},
 	{

Hi Neil,

As you point out that in "[PATCH] sctp: make the max_burst min value to 1":

Quoted 2 lines

1) You can also set the the max_burst via setsockopt, and so this would need to be checked in that path as well.

we can set the rto_min/rto_max via setsockopt. Should I check it in the path as well?

Thanks.

Vlad Yasevich

2013-12-02 14:45 UTC #2 lore raw

In reply to Wang Weidong

Quoted 24 lines · Wang Weidong

rto_min should be smaller than rto_max while rto_max should be larger than rto_min. so just add the check.
Acked-by Neil Horman <[email protected]>
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/sctp/sysctl.c +1 -1
index 6b36561..7637e8e 100644
@@ -104,7 +104,7 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1         = &one,
-		.extra2         = &timer_max
+		.extra2         = &init_net.sctp.rto_max
 	},
 	{
 		.procname	= "rto_max",

net/sctp/sysctl.c static struct ctl_table sctp_net_table[] = { · line 118

Quoted 8 lines

net/sctp/sysctl.c +1 -1

@@ -112,7 +112,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
+		.extra1         = &init_net.sctp.rto_min,
 		.extra2         = &timer_max
 	},
 	{

So, now if initial namespace change rto_min and rto_max, it limits the values that another namespace can set? I don't think so.

If you are looking to fix an issue where rto_max may be set below rto_min, then do a separate proc_handler.

-vlad

Wang Weidong

author

2013-12-03 01:58 UTC #3 lore raw

Quoted 28 lines · Vlad Yasevich

Quoted 27 lines · Wang Weidong

rto_min should be smaller than rto_max while rto_max should be larger than rto_min. so just add the check.

Acked-by Neil Horman <[email protected]>

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/sysctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/sctp/sysctl.c +1 -2

index 6b36561..7637e8e 100644
@@ -104,7 +104,7 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1         = &one,
-		.extra2         = &timer_max
+		.extra2         = &init_net.sctp.rto_max
 	},
 	{
 		.procname	= "rto_max",

@@ -112,7 +112,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,

Quoted 8 lines

net/sctp/sysctl.c static struct ctl_table sctp_net_table[] = { · line 118
Quoted 5 lines
net/sctp/sysctl.c +1 -0
+		.extra1         = &init_net.sctp.rto_min,
 		.extra2         = &timer_max
 	},
 	{
So, now if initial namespace change rto_min and rto_max, it limits the values that another namespace can set? I don't think so.

Hm, you are right.
I think we have two path to set the rto_min or rto_max: 1)systclt, 2)setsockopt In 2) we can make the assoc or sk rto_max smaller than rto_min. so we should a check in the sctp_setsockopt_rtoinfo or proc_handler.

Quoted 3 lines

If you are looking to fix an issue where rto_max may be set below rto_min, then do a separate proc_handler.

I will try to add a separate proc_handler.
Thanks.

Quoted 8 lines

-vlad
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wang Weidong

author

2013-12-03 06:28 UTC #4 lore raw

PATCH v3 1/1 sctp: check the rto_min and rto_max

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.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/socket.c |  5 +++++
 net/sctp/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 7 deletions(-)

net/sctp/socket.c +5 -0

index 72046b9..2e1af1b 100644
@@ -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 */

net/sctp/sysctl.c +51 -7

83 lines not on this page · raw message

Signature · 1 line

1.7.12

Vlad Yasevich

2013-12-03 12:39 UTC #5 lore raw

Quoted 104 lines · 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.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/socket.c |  5 +++++
 net/sctp/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 7 deletions(-)

net/sctp/socket.c +5 -0

index 72046b9..2e1af1b 100644
@@ -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 */

net/sctp/sysctl.c +41 -7

index 6b36561..525b06d 100644
@@ -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_check_rtomin(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+static int proc_sctp_check_rtomax(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,18 +110,14 @@ static struct ctl_table sctp_net_table[] = {
 		.data		= &init_net.sctp.rto_min,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
-		.extra2         = &timer_max
+		.proc_handler	= proc_sctp_check_rtomin,
 	},
 	{
 		.procname	= "rto_max",
 		.data		= &init_net.sctp.rto_max,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
-		.extra2         = &timer_max
+		.proc_handler	= proc_sctp_check_rtomax,
 	},
 	{
 		.procname	= "rto_alpha_exp_divisor",

@@ -342,6 +346,46 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 	return ret;
 }
 
+static int proc_sctp_check_rtomin(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	int old_value = *(int *) ctl->data;
+	int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+
+	if (write) {
+		int new_value = *(int *) ctl->data;
+		if (ret || new_value < one || new_value > init_net.sctp.rto_max) {
+			init_net.sctp.rto_min = old_value;
+			return -EINVAL;
+		}
+		init_net.sctp.rto_min = new_value;
+	}
+
+	return ret;
+}
+
+static int proc_sctp_check_rtomax(struct ctl_table *ctl,
+				int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	int old_value = *(int *) ctl->data;
+	int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+
+	if (write) {
+		int new_value = *(int *) ctl->data;

net/sctp/sysctl.c static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, · line 388

Quoted 10 lines

net/sctp/sysctl.c +10 -0

+		if (ret || new_value > timer_max || new_value < init_net.sctp.rto_min) {
+			init_net.sctp.rto_max = old_value;
+			return -EINVAL;
+		}
+		init_net.sctp.rto_max = new_value;
+	}
+
+	return ret;
+}
+

You can't used init_net directly. You have to use .data as the target of assignment and .extra1 and .extra2 as the min and max boundries. Otherwise, this has the same problem as before where assignments for created namespace try to change the values in the initial namespace.

-vlad

Quoted 4 lines

net/sctp/sysctl.c +0 -0

 int sctp_sysctl_net_register(struct net *net)
 {
 	struct ctl_table *table;

Wang Weidong

author

2013-12-04 02:46 UTC #6 lore raw

Quoted 122 lines · Vlad Yasevich

Quoted 104 lines · 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.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/socket.c |  5 +++++
 net/sctp/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 7 deletions(-)

net/sctp/socket.c +5 -0

index 72046b9..2e1af1b 100644
@@ -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 */

net/sctp/sysctl.c +41 -7

index 6b36561..525b06d 100644
@@ -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_check_rtomin(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+static int proc_sctp_check_rtomax(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,18 +110,14 @@ static struct ctl_table sctp_net_table[] = {
 		.data		= &init_net.sctp.rto_min,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
-		.extra2         = &timer_max
+		.proc_handler	= proc_sctp_check_rtomin,
 	},
 	{
 		.procname	= "rto_max",
 		.data		= &init_net.sctp.rto_max,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
-		.extra2         = &timer_max
+		.proc_handler	= proc_sctp_check_rtomax,
 	},
 	{
 		.procname	= "rto_alpha_exp_divisor",

@@ -342,6 +346,46 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 	return ret;
 }
 
+static int proc_sctp_check_rtomin(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	int old_value = *(int *) ctl->data;
+	int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+
+	if (write) {
+		int new_value = *(int *) ctl->data;
+		if (ret || new_value < one || new_value > init_net.sctp.rto_max) {
+			init_net.sctp.rto_min = old_value;
+			return -EINVAL;
+		}
+		init_net.sctp.rto_min = new_value;
+	}
+
+	return ret;
+}
+
+static int proc_sctp_check_rtomax(struct ctl_table *ctl,
+				int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	int old_value = *(int *) ctl->data;
+	int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+
+	if (write) {
+		int new_value = *(int *) ctl->data;

net/sctp/sysctl.c static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, · line 388

Quoted 10 lines

net/sctp/sysctl.c +10 -0

+		if (ret || new_value > timer_max || new_value < init_net.sctp.rto_min) {
+			init_net.sctp.rto_max = old_value;
+			return -EINVAL;
+		}
+		init_net.sctp.rto_max = new_value;
+	}
+
+	return ret;
+}
+

Quoted 1 line

-vlad

Hi, Vlad

Do You mean like below?

For rto_max:
.procname = "rto_max",
.data = &init_net.sctp.rto_max,
.maxlen = sizeof(unsigned int),
.mode = 0644,
.proc_handler = &proc_sctp_check_rtomax,
.extra1 = &init_net.sctp.rto_min,
.extra2 = &timer_max

proc_sctp_check_rtomax()

        *net = current->nsproxy->net_ns;
        min = *(unsigned int *) ctl->extra1;
        max = *(unsigned int *) ctl->extra2;
	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;
	}

and as the same as rto_min.
Thanks!

Quoted 12 lines

net/sctp/sysctl.c static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, · line 391
Quoted 4 lines
net/sctp/sysctl.c +0 -0
 int sctp_sysctl_net_register(struct net *net)
 {
 	struct ctl_table *table;
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
.

Vlad Yasevich

2013-12-04 14:24 UTC #7 lore raw

Quoted 154 lines · Wang Weidong

Quoted 122 lines · Vlad Yasevich

Quoted 104 lines · 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.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/socket.c |  5 +++++
 net/sctp/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 7 deletions(-)

net/sctp/socket.c +5 -0

index 72046b9..2e1af1b 100644
@@ -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 */

net/sctp/sysctl.c +41 -7

index 6b36561..525b06d 100644
@@ -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_check_rtomin(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+static int proc_sctp_check_rtomax(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,18 +110,14 @@ static struct ctl_table sctp_net_table[] = {
 		.data		= &init_net.sctp.rto_min,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
-		.extra2         = &timer_max
+		.proc_handler	= proc_sctp_check_rtomin,
 	},
 	{
 		.procname	= "rto_max",
 		.data		= &init_net.sctp.rto_max,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
-		.extra2         = &timer_max
+		.proc_handler	= proc_sctp_check_rtomax,
 	},
 	{
 		.procname	= "rto_alpha_exp_divisor",

@@ -342,6 +346,46 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 	return ret;
 }
 
+static int proc_sctp_check_rtomin(struct ctl_table *ctl,
+				int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	int old_value = *(int *) ctl->data;
+	int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+
+	if (write) {
+		int new_value = *(int *) ctl->data;
+		if (ret || new_value < one || new_value > init_net.sctp.rto_max) {
+			init_net.sctp.rto_min = old_value;
+			return -EINVAL;
+		}
+		init_net.sctp.rto_min = new_value;
+	}
+
+	return ret;
+}
+
+static int proc_sctp_check_rtomax(struct ctl_table *ctl,
+				int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	int old_value = *(int *) ctl->data;
+	int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+
+	if (write) {
+		int new_value = *(int *) ctl->data;

net/sctp/sysctl.c static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, · line 388

Quoted 10 lines

net/sctp/sysctl.c +10 -0

+		if (ret || new_value > timer_max || new_value < init_net.sctp.rto_min) {
+			init_net.sctp.rto_max = old_value;
+			return -EINVAL;
+		}
+		init_net.sctp.rto_max = new_value;
+	}
+
+	return ret;
+}
+

Quoted 1 line

-vlad

Hi, Vlad

Do You mean like below?

proc_sctp_check_rtomax()

        *net = current->nsproxy->net_ns;
        min = *(unsigned int *) ctl->extra1;
        max = *(unsigned int *) ctl->extra2;
	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;
	}

Quoted 3 lines

and as the same as rto_min.
Thanks!

That looks about right.

-vlad

Quoted 14 lines

Quoted 12 lines
net/sctp/sysctl.c static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, · line 391
Quoted 4 lines
net/sctp/sysctl.c +0 -0
 int sctp_sysctl_net_register(struct net *net)
 {
 	struct ctl_table *table;
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
.