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

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

[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 <shanwei@cn.fujitsu.com>
---
 net/ipv6/ipv6_sockglue.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4e5eac3..7a58597 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -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;
-- 
1.6.0

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[David Miller]

From: Shan Wei <shanwei@cn.fujitsu.com>
Date: Wed, 03 Sep 2008 16:41:06 +0800

>  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 <shanwei@cn.fujitsu.com>

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.

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[Shan Wei]

David Miller 写道:
> From: Shan Wei <shanwei@cn.fujitsu.com>
> Date: Wed, 03 Sep 2008 16:41:06 +0800
> 
>>  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 <shanwei@cn.fujitsu.com>
> 
> 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.

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[Brian Haley]

Shan Wei wrote:
>  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 <shanwei@cn.fujitsu.com>
> ---
>  net/ipv6/ipv6_sockglue.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 4e5eac3..7a58597 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -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...

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

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[Shan Wei]

Brian Haley 写道:
> Shan Wei wrote:
>>  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 <shanwei@cn.fujitsu.com>
>> ---
>>  net/ipv6/ipv6_sockglue.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
>> index 4e5eac3..7a58597 100644
>> --- a/net/ipv6/ipv6_sockglue.c
>> +++ b/net/ipv6/ipv6_sockglue.c
>> @@ -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...
> 
For uncommon expection, if the check is true, we set the error. 
it will reduce the number of setting retv value.

>> @@ -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.
> 
yes 
> 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
> 

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[David Miller]

From: Shan Wei <shanwei@cn.fujitsu.com>
Date: Thu, 04 Sep 2008 11:44:10 +0800

> Brian Haley 写道:
> > Shan Wei wrote:
> >> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> >> index 4e5eac3..7a58597 100644
> >> --- a/net/ipv6/ipv6_sockglue.c
> >> +++ b/net/ipv6/ipv6_sockglue.c
> >> @@ -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...
> > 
> 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.

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[Shan Wei]

David Miller 写道:
> From: Shan Wei <shanwei@cn.fujitsu.com>
> Date: Thu, 04 Sep 2008 11:44:10 +0800
> 
>> Brian Haley 写道:
>>> Shan Wei wrote:
>>>> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
>>>> index 4e5eac3..7a58597 100644
>>>> --- a/net/ipv6/ipv6_sockglue.c
>>>> +++ b/net/ipv6/ipv6_sockglue.c
>>>> @@ -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...
>>>
>> 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.
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.

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[Shan Wei]

Brian Haley 写道:
> Shan Wei wrote:

...snip...

> 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 <shanwei@cn.fujitsu.com>
---
 net/ipv6/datagram.c      |    8 ++++++--
 net/ipv6/ipv6_sockglue.c |    7 +++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 410046a..cfd499a 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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;
 			}
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4e5eac3..b967965 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -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;
 		}
 
-- 
1.6.0

Re: [PATCH RESEND] ipv6: return with appropriate error code when sending RH0 using setsockopt()

[Brian Haley]

Shan Wei wrote:
> Brian Haley 写道:
>> Shan Wei wrote:
> 
> ...snip...
> 
>> 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 <shanwei@cn.fujitsu.com>
> ---
>  net/ipv6/datagram.c      |    8 ++++++--
>  net/ipv6/ipv6_sockglue.c |    7 +++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 410046a..cfd499a 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -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;
>  			}
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 4e5eac3..b967965 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -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;
>  		}

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