[Patch] net: fix an array index overflow

[Patch] net: fix an array index overflow

[Amerigo Wang]

Don't use the address of an out-of-boundary element.

Maybe this is not harmful at runtime, but it is still
good to improve it.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: David S. Miller <davem@davemloft.net>

---
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 57737b8..2669361 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1586,7 +1586,7 @@ static int __init inet_init(void)
 #endif
 
 	/* Register the socket-side information for inet_create. */
-	for (r = &inetsw[0]; r < &inetsw[SOCK_MAX]; ++r)
+	for (r = &inetsw[0]; r <= &inetsw[SOCK_MAX-1]; ++r)
 		INIT_LIST_HEAD(r);
 
 	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)

Re: [Patch] net: fix an array index overflow

[Eric Dumazet]

Amerigo Wang a écrit :
> Don't use the address of an out-of-boundary element.
> 
> Maybe this is not harmful at runtime, but it is still
> good to improve it.

Why ?

for (ptr = start; ptr < end; ptr++) {}

is valid, even if 'end' is 'outside of bounds'

It also works if start == end.

> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> 
> ---
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 57737b8..2669361 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1586,7 +1586,7 @@ static int __init inet_init(void)
>  #endif
>  
>  	/* Register the socket-side information for inet_create. */
> -	for (r = &inetsw[0]; r < &inetsw[SOCK_MAX]; ++r)
> +	for (r = &inetsw[0]; r <= &inetsw[SOCK_MAX-1]; ++r)
>  		INIT_LIST_HEAD(r);
>  
>  	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)
> --

I wonder why you want to 'fix' this loop and let following loop unchanged...

	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)
		inet_register_protosw(q);

If this really hurts your eyes, why not using basic loops ?

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 7d12c6a..476cda7 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1540,8 +1540,7 @@ static struct packet_type ip_packet_type __read_mostly = {
 static int __init inet_init(void)
 {
 	struct sk_buff *dummy_skb;
-	struct inet_protosw *q;
-	struct list_head *r;
+	int i;
 	int rc = -EINVAL;
 
 	BUILD_BUG_ON(sizeof(struct inet_skb_parm) > sizeof(dummy_skb->cb));
@@ -1584,11 +1583,11 @@ static int __init inet_init(void)
 #endif
 
 	/* Register the socket-side information for inet_create. */
-	for (r = &inetsw[0]; r < &inetsw[SOCK_MAX]; ++r)
-		INIT_LIST_HEAD(r);
+	for (i = 0; i < SOCK_MAX; i++)
+		INIT_LIST_HEAD(&inetsw[i]);
 
-	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)
-		inet_register_protosw(q);
+	for (i = 0; i < INETSW_ARRAY_LEN; i++)
+		inet_register_protosw(&inetsw_array[i]);
 
 	/*
 	 *	Set the ARP module up

Re: [Patch] net: fix an array index overflow

[Cong Wang]

Eric Dumazet wrote:
> Amerigo Wang a écrit :
>> Don't use the address of an out-of-boundary element.
>>
>> Maybe this is not harmful at runtime, but it is still
>> good to improve it.
> 
> Why ?
> 
> for (ptr = start; ptr < end; ptr++) {}
> 
> is valid, even if 'end' is 'outside of bounds'
> 
> It also works if start == end.

I knew it's valid, that is why I said it's "not harmful".

> 
>> Signed-off-by: WANG Cong <amwang@redhat.com>
>> Cc: David S. Miller <davem@davemloft.net>
>>
>> ---
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index 57737b8..2669361 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -1586,7 +1586,7 @@ static int __init inet_init(void)
>>  #endif
>>  
>>  	/* Register the socket-side information for inet_create. */
>> -	for (r = &inetsw[0]; r < &inetsw[SOCK_MAX]; ++r)
>> +	for (r = &inetsw[0]; r <= &inetsw[SOCK_MAX-1]; ++r)
>>  		INIT_LIST_HEAD(r);
>>  
>>  	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)
>> --
> 
> I wonder why you want to 'fix' this loop and let following loop unchanged...
> 
> 	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)
> 		inet_register_protosw(q);
> 


Oh, I didn't catch it.

> If this really hurts your eyes, why not using basic loops ?


Yes, definitely this one is better.
Ack.


> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 7d12c6a..476cda7 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1540,8 +1540,7 @@ static struct packet_type ip_packet_type __read_mostly = {
>  static int __init inet_init(void)
>  {
>  	struct sk_buff *dummy_skb;
> -	struct inet_protosw *q;
> -	struct list_head *r;
> +	int i;
>  	int rc = -EINVAL;
>  
>  	BUILD_BUG_ON(sizeof(struct inet_skb_parm) > sizeof(dummy_skb->cb));
> @@ -1584,11 +1583,11 @@ static int __init inet_init(void)
>  #endif
>  
>  	/* Register the socket-side information for inet_create. */
> -	for (r = &inetsw[0]; r < &inetsw[SOCK_MAX]; ++r)
> -		INIT_LIST_HEAD(r);
> +	for (i = 0; i < SOCK_MAX; i++)
> +		INIT_LIST_HEAD(&inetsw[i]);
>  
> -	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)
> -		inet_register_protosw(q);
> +	for (i = 0; i < INETSW_ARRAY_LEN; i++)
> +		inet_register_protosw(&inetsw_array[i]);
>  
>  	/*
>  	 *	Set the ARP module up

Re: [Patch] net: fix an array index overflow

[Dan Carpenter]

On Tue, Dec 01, 2009 at 03:26:02AM -0500, Amerigo Wang wrote:
> 
> Don't use the address of an out-of-boundary element.
> 
> Maybe this is not harmful at runtime, but it is still
> good to improve it.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> 

It may be coincidence but my static checker smatch also complains 
about the code you modified.

It's the wrong idea to fix code to please a checker.  You end up
doing things like adding an extra "return -ENOTREACHED" to silence
warnings.  Then the next person who writes a checker has to figure
out how to seperate the unreachable code which was added to suppress
gcc warnings from bits which are unreachable because of typos.

Really any code that a human can read, a static checker should also
be able to read.  Computer programs are just state machines.  At 
the function level they are quite small state machines.  It's all
logic and math which computers are very good at.  So it should be 
fairly easy to fix the checker.  ;)

(The above paragraph is funnier if you knew how sucky smatch is).

regards,
dan carpenter

> ---
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 57737b8..2669361 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1586,7 +1586,7 @@ static int __init inet_init(void)
>  #endif
>  
>  	/* Register the socket-side information for inet_create. */
> -	for (r = &inetsw[0]; r < &inetsw[SOCK_MAX]; ++r)
> +	for (r = &inetsw[0]; r <= &inetsw[SOCK_MAX-1]; ++r)
>  		INIT_LIST_HEAD(r);
>  
>  	for (q = inetsw_array; q < &inetsw_array[INETSW_ARRAY_LEN]; ++q)

Re: [Patch] net: fix an array index overflow

[Cong Wang]

Dan Carpenter wrote:
> On Tue, Dec 01, 2009 at 03:26:02AM -0500, Amerigo Wang wrote:
>> Don't use the address of an out-of-boundary element.
>>
>> Maybe this is not harmful at runtime, but it is still
>> good to improve it.
>>
>> Signed-off-by: WANG Cong <amwang@redhat.com>
>> Cc: David S. Miller <davem@davemloft.net>
>>
> 
> It may be coincidence but my static checker smatch also complains 
> about the code you modified.
> 
> It's the wrong idea to fix code to please a checker.  You end up
> doing things like adding an extra "return -ENOTREACHED" to silence
> warnings.  Then the next person who writes a checker has to figure
> out how to seperate the unreachable code which was added to suppress
> gcc warnings from bits which are unreachable because of typos.
> 
> Really any code that a human can read, a static checker should also
> be able to read.  Computer programs are just state machines.  At 
> the function level they are quite small state machines.  It's all
> logic and math which computers are very good at.  So it should be 
> fairly easy to fix the checker.  ;)
> 

Well, in some cases smatch seems really wrong, but not in this
case I think, or at least, smatch is suggesting us to improve
this code.

Please check Eric's reply in this thread, his patch looks nice
for me.

Thanks.