ipv6: return with appropriate error code when sending RH0 using setsockopt()

PATCH RESEND In review Shan Wei 3 participants 9 messages Sep 3, 2008 – Sep 16, 2008

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Shan Wei

author

2008-09-03 08:43 UTC #0 lore raw

PATCH RESEND 1/1 ipv6: return with appropriate error code when sending RH0 using setsockopt()

I'm sorry to resend the patch, for that no one replied it from Jun 27.

The kernel had removed the RH0(Type 0 Routing Header), but we can still send IPv6 packet with RH0 using sendmsg() or setsockopt().

Compare with sendmsg() that returns EINVAL, but setsockopt() return EPERM. The patch fix it.

Signed-off-by Shan Wei <[email protected]>

---
 net/ipv6/ipv6_sockglue.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

net/ipv6/ipv6_sockglue.c +4 -2

index 4e5eac3..7a58597 100644
@@ -353,9 +353,10 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			goto e_inval;
 
 		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW))
+		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW)) {
+			retv = -EPERM;
 			break;
+		}
 
 		opt = ipv6_renew_options(sk, np->opt, optname,
 					 (struct ipv6_opt_hdr __user *)optval,

@@ -365,6 +366,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			break;
 		}
 
+		retv = -EINVAL;
 		/* routing header option needs extra check */
 		if (optname == IPV6_RTHDR && opt && opt->srcrt) {
 			struct ipv6_rt_hdr *rthdr = opt->srcrt;

Signature · 1 line

1.6.0

David Miller

maintainer

2008-09-03 09:23 UTC #1 lore raw

From: Shan Wei <[email protected]>
Date: Wed, 03 Sep 2008 16:41:06 +0800

Quoted 9 lines

I'm sorry to resend the patch, for that no one replied it from Jun 27.
The kernel had removed the RH0(Type 0 Routing Header), but we can still send IPv6 packet with RH0 using sendmsg() or setsockopt().
Compare with sendmsg() that returns EINVAL, but setsockopt() return EPERM. The patch fix it.
Signed-off-by Shan Wei <[email protected]>

I've tried to defer IPV6 patches that have semantic issues to an expert such as Yoshifuji-san, but he has dropped off the face of the planet for a while.

I'll take a look at this if I get a chance.

Shan Wei

author

2008-09-03 09:43 UTC #2 lore raw

Quoted 11 lines · David Miller

From: Shan Wei <[email protected]>
Date: Wed, 03 Sep 2008 16:41:06 +0800
Quoted 8 lines
I'm sorry to resend the patch, for that no one replied it from Jun 27.
The kernel had removed the RH0(Type 0 Routing Header), but we can still send IPv6 packet with RH0 using sendmsg() or setsockopt().
Compare with sendmsg() that returns EINVAL, but setsockopt() return EPERM. The patch fix it.

Quoted 8 lines

Quoted 1 line
Signed-off-by Shan Wei <[email protected]>
I've tried to defer IPV6 patches that have semantic issues to an expert such as Yoshifuji-san, but he has dropped off the face of the planet for a while.
I'll take a look at this if I get a chance.

thanks for your so fast reply。
I'm patient to wait for your ack.

Brian Haley

2008-09-03 18:36 UTC #3 lore raw

In reply to Shan Wei

Quoted 18 lines · Shan Wei

I'm sorry to resend the patch, for that no one replied it from Jun 27.
The kernel had removed the RH0(Type 0 Routing Header), but we can still send IPv6 packet with RH0 using sendmsg() or setsockopt().
Compare with sendmsg() that returns EINVAL, but setsockopt() return EPERM. The patch fix it.
Signed-off-by Shan Wei <[email protected]>
---
 net/ipv6/ipv6_sockglue.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
net/ipv6/ipv6_sockglue.c +0 -0
index 4e5eac3..7a58597 100644

net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 359

Quoted 9 lines

net/ipv6/ipv6_sockglue.c +3 -2

@@ -353,9 +353,10 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			goto e_inval;
 
 		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW))
+		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW)) {
+			retv = -EPERM;
 			break;
+		}

I'm not sure you need this since it doesn't actually change anything, setting the error before the check just seems like the style in this function. It will get set to -EINVAL below...

net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 372

Quoted 8 lines

net/ipv6/ipv6_sockglue.c +1 -0

@@ -365,6 +366,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			break;
 		}
 
+		retv = -EINVAL;
 		/* routing header option needs extra check */
 		if (optname == IPV6_RTHDR && opt && opt->srcrt) {
 			struct ipv6_rt_hdr *rthdr = opt->srcrt;

This seems correct as trying to set an unsupported routing header type is invalid, or trying to set a Type 2 that itself is invalid is bad. The Unix I checked does this.

There's actually another bug here and in the sendmsg() path in that you can set the hdrlen and segments_left fields to be invalid (according to RFC3775), as long as the math works out (segments * 2 == length). Segments_left should always be 1 and hdrlen 2 for a Type2 routing header. The packet should be dropped at the destination, but we probably shouldn't send it. I can send a patch for that later.

-Brian

Shan Wei

author

2008-09-04 03:46 UTC #4 lore raw

Quoted 21 lines · Brian Haley

Quoted 20 lines · Shan Wei
I'm sorry to resend the patch, for that no one replied it from Jun 27.
The kernel had removed the RH0(Type 0 Routing Header), but we can still send IPv6 packet with RH0 using sendmsg() or setsockopt().
Compare with sendmsg() that returns EINVAL, but setsockopt() return EPERM. The patch fix it.
Signed-off-by Shan Wei <[email protected]>
---
 net/ipv6/ipv6_sockglue.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
net/ipv6/ipv6_sockglue.c +0 -0
index 4e5eac3..7a58597 100644
@@ -353,9 +353,10 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			goto e_inval;
 

Quoted 12 lines

net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 359
Quoted 7 lines
net/ipv6/ipv6_sockglue.c +3 -2
 		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW))
+		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW)) {
+			retv = -EPERM;
 			break;
+		}
I'm not sure you need this since it doesn't actually change anything, setting the error before the check just seems like the style in this function. It will get set to -EINVAL below...

For uncommon expection, if the check is true, we set the error. it will reduce the number of setting retv value.

Quoted 4 lines

net/ipv6/ipv6_sockglue.c +0 -0

@@ -365,6 +366,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			break;
 		}

Quoted 9 lines

net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 372
Quoted 4 lines
net/ipv6/ipv6_sockglue.c +1 -0
+		retv = -EINVAL;
 		/* routing header option needs extra check */
 		if (optname == IPV6_RTHDR && opt && opt->srcrt) {
 			struct ipv6_rt_hdr *rthdr = opt->srcrt;
This seems correct as trying to set an unsupported routing header type is invalid, or trying to set a Type 2 that itself is invalid is bad. The Unix I checked does this.

yes

Quoted 9 lines

There's actually another bug here and in the sendmsg() path in that you can set the hdrlen and segments_left fields to be invalid (according to RFC3775), as long as the math works out (segments * 2 == length). Segments_left should always be 1 and hdrlen 2 for a Type2 routing header. The packet should be dropped at the destination, but we probably shouldn't send it. I can send a patch for that later.
-Brian

David Miller

maintainer

2008-09-12 03:15 UTC #5 lore raw

From: Shan Wei <[email protected]>
Date: Thu, 04 Sep 2008 11:44:10 +0800

Quoted 21 lines

Quoted 8 lines · Brian Haley
Quoted 7 lines · Shan Wei
net/ipv6/ipv6_sockglue.c +0 -0
index 4e5eac3..7a58597 100644
@@ -353,9 +353,10 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			goto e_inval;
 
Quoted 12 lines
net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 359
Quoted 7 lines
net/ipv6/ipv6_sockglue.c +3 -2
 		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW))
+		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW)) {
+			retv = -EPERM;
 			break;
+		}
I'm not sure you need this since it doesn't actually change anything, setting the error before the check just seems like the style in this function. It will get set to -EINVAL below...

Quoted 2 lines

For uncommon expection, if the check is true, we set the error. it will reduce the number of setting retv value.

Do you have some emotional attachment to this change? :-/

The compiler optimizer will make the transformation you claim is now possible, all by itself, if it is deemed more optimal.

Never make any claims about how many times something will or will not happen in the compiled output without consulting and providing examples of the resulting assembler. Otherwise you are just guessing.

This is just a pointless change, and as Brian said it disagrees with the way this construct is made in the rest of this function. So please take it out of your patch.

Shan Wei

author

2008-09-16 07:15 UTC #6 lore raw

Quoted 31 lines · David Miller

From: Shan Wei <[email protected]>
Date: Thu, 04 Sep 2008 11:44:10 +0800
Quoted 20 lines
Quoted 8 lines · Brian Haley
Quoted 7 lines · Shan Wei
net/ipv6/ipv6_sockglue.c +0 -0
index 4e5eac3..7a58597 100644
@@ -353,9 +353,10 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			goto e_inval;
 
Quoted 11 lines
net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 359
Quoted 7 lines
net/ipv6/ipv6_sockglue.c +3 -2
 		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW))
+		if (optname != IPV6_RTHDR && !capable(CAP_NET_RAW)) {
+			retv = -EPERM;
 			break;
+		}
I'm not sure you need this since it doesn't actually change anything, setting the error before the check just seems like the style in this function. It will get set to -EINVAL below...
Quoted 2 lines
For uncommon expection, if the check is true, we set the error. it will reduce the number of setting retv value.
Do you have some emotional attachment to this change? :-/
The compiler optimizer will make the transformation you claim is now possible, all by itself, if it is deemed more optimal.

Quoted 7 lines

Never make any claims about how many times something will or will not happen in the compiled output without consulting and providing examples of the resulting assembler. Otherwise you are just guessing.
This is just a pointless change, and as Brian said it disagrees with the way this construct is made in the rest of this function. So please take it out of your patch.

sorry for repling late.
maybe what i describe is not explicit.

in this function the style is like this:
case XX:
{
retv = -EPROTO;
if (condition1) break;
...
retv = -EFAULT;
if (condition2) break;
...
retv = -EINVAL;
if (condition3) break;
...
}
when the conditions are all false, the retv value is set three times.

but with the following style, the retv value is not set. case XX:
{
if (condition1){ retv = -EPROTO;break;}
...
if (condition2){ retv = -EFAULT;break;}
...
if (condition3){ retv = -EINVAL;break;}
...
}
Seting the retv value when expection is occurred almost don't improve the performance, but it's a little better.

Shan Wei

author

2008-09-04 05:55 UTC #7 lore raw

In reply to Brian Haley

Quoted 1 line · Brian Haley

Shan Wei wrote:

...snip...

Quoted 7 lines

There's actually another bug here and in the sendmsg() path in that you can set the hdrlen and segments_left fields to be invalid (according to RFC3775), as long as the math works out (segments * 2 == length). Segments_left should always be 1 and hdrlen 2 for a Type2 routing header. The packet should be dropped at the destination, but we probably shouldn't send it. I can send a patch for that later.

I check the RFC3775. May be the following patch is good to you.

Signed-off-by Shan Wei <[email protected]>

---
 net/ipv6/datagram.c      |    8 ++++++--
 net/ipv6/ipv6_sockglue.c |    7 +++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

net/ipv6/datagram.c +6 -2

index 410046a..cfd499a 100644
@@ -675,8 +675,12 @@ int datagram_send_ctl(struct net *net,
 				goto exit_f;
 			}
 
-			/* segments left must also match */
-			if ((rthdr->hdrlen >> 1) != rthdr->segments_left) {
+			/* Per RFC3775, for a type 2 routing header,
+			 * the Hdr Ext Len MUST be 2 and the Segments Left
+			 * MUST be 1.
+			 */
+			if ((rthdr->hdrlen != 2) &&
+			    (rthdr->segments_left != 1)) {
 				err = -EINVAL;
 				goto exit_f;
 			}

net/ipv6/ipv6_sockglue.c +5 -2

index 4e5eac3..b967965 100644
@@ -377,8 +377,11 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 				goto sticky_done;
 			}
 
-			if ((rthdr->hdrlen & 1) ||
-			    (rthdr->hdrlen >> 1) != rthdr->segments_left)
+			/* Per RFC3775, for a type 2 routing header,
+			 * the Hdr Ext Len MUST be 2 and the Segments Left
+			 * MUST be 1.
+			 */
+			if ((rthdr->hdrlen != 2) && (rthdr->segments_left != 1))
 				goto sticky_done;
 		}

Signature · 1 line

1.6.0

Brian Haley

2008-09-04 13:54 UTC #8 lore raw

Quoted 46 lines · Shan Wei

Quoted 1 line · Brian Haley
Shan Wei wrote:
...snip...
Quoted 7 lines
There's actually another bug here and in the sendmsg() path in that you can set the hdrlen and segments_left fields to be invalid (according to RFC3775), as long as the math works out (segments * 2 == length). Segments_left should always be 1 and hdrlen 2 for a Type2 routing header. The packet should be dropped at the destination, but we probably shouldn't send it. I can send a patch for that later.
I check the RFC3775. May be the following patch is good to you.
Signed-off-by Shan Wei <[email protected]>
---
 net/ipv6/datagram.c      |    8 ++++++--
 net/ipv6/ipv6_sockglue.c |    7 +++++--
 2 files changed, 11 insertions(+), 4 deletions(-)
net/ipv6/datagram.c +6 -2
index 410046a..cfd499a 100644
@@ -675,8 +675,12 @@ int datagram_send_ctl(struct net *net,
 				goto exit_f;
 			}
 
-			/* segments left must also match */
-			if ((rthdr->hdrlen >> 1) != rthdr->segments_left) {
+			/* Per RFC3775, for a type 2 routing header,
+			 * the Hdr Ext Len MUST be 2 and the Segments Left
+			 * MUST be 1.
+			 */
+			if ((rthdr->hdrlen != 2) &&
+			    (rthdr->segments_left != 1)) {
 				err = -EINVAL;
 				goto exit_f;
 			}
net/ipv6/ipv6_sockglue.c +0 -0
index 4e5eac3..b967965 100644
@@ -377,8 +377,11 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 				goto sticky_done;
 			}

net/ipv6/ipv6_sockglue.c static int do_ipv6_setsockopt(struct sock *sk, int level, int optname, · line 386

Quoted 10 lines

net/ipv6/ipv6_sockglue.c +5 -2

 
-			if ((rthdr->hdrlen & 1) ||
-			    (rthdr->hdrlen >> 1) != rthdr->segments_left)
+			/* Per RFC3775, for a type 2 routing header,
+			 * the Hdr Ext Len MUST be 2 and the Segments Left
+			 * MUST be 1.
+			 */
+			if ((rthdr->hdrlen != 2) && (rthdr->segments_left != 1))
 				goto sticky_done;
 		}

I think these checks need to be in the Type 2 specific code since if we add support for Type 7, for example, then we'd have to change this again. I"'ll send something out soon.

-Brian