[PATCH] eexpress: Read buffer overflow

[PATCH] eexpress: Read buffer overflow

[Roel Kluin]

start_code is 69 words, but the code always writes a multiple of 16 words,
so the last 11 words written are outside the array.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
This was observed using Parfait http://research.sun.com/projects/parfait/

diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
index 1686dca..e304abb 100644
--- a/drivers/net/eexpress.c
+++ b/drivers/net/eexpress.c
@@ -1474,15 +1474,14 @@ static void eexp_hw_init586(struct net_device *dev)
 	outw(0x0000, ioaddr + 0x800c);
 	outw(0x0000, ioaddr + 0x800e);
 
-	for (i = 0; i < (sizeof(start_code)); i+=32) {
-		int j;
-		outw(i, ioaddr + SM_PTR);
-		for (j = 0; j < 16; j+=2)
-			outw(start_code[(i+j)/2],
-			     ioaddr+0x4000+j);
-		for (j = 0; j < 16; j+=2)
-			outw(start_code[(i+j+16)/2],
-			     ioaddr+0x8000+j);
+	for (i = 0; i < ARRAY_SIZE(start_code); i += 16) {
+		int j, jmax;
+		outw(i * 2, ioaddr + SM_PTR);
+
+		jmax = min_t(int, 16, ARRAY_SIZE(start_code) - i);
+		for (j = 0; j < jmax; j++)
+			outw(start_code[i + j],
+			     ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16));
 	}
 
 	/* Do we want promiscuous mode or multicast? */

Re: [PATCH] eexpress: Read buffer overflow

[Jarek Poplawski]

Roel Kluin wrote, On 07/25/2009 09:50 PM:

> start_code is 69 words, but the code always writes a multiple of 16 words,
> so the last 11 words written are outside the array.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> This was observed using Parfait http://research.sun.com/projects/parfait/
> 
> diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
> index 1686dca..e304abb 100644
> --- a/drivers/net/eexpress.c
> +++ b/drivers/net/eexpress.c
> @@ -1474,15 +1474,14 @@ static void eexp_hw_init586(struct net_device *dev)
>  	outw(0x0000, ioaddr + 0x800c);
>  	outw(0x0000, ioaddr + 0x800e);
>  
> -	for (i = 0; i < (sizeof(start_code)); i+=32) {
> -		int j;
> -		outw(i, ioaddr + SM_PTR);
> -		for (j = 0; j < 16; j+=2)
> -			outw(start_code[(i+j)/2],
> -			     ioaddr+0x4000+j);
> -		for (j = 0; j < 16; j+=2)
> -			outw(start_code[(i+j+16)/2],
> -			     ioaddr+0x8000+j);


(max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
less than its size?

Jarek P.

> +	for (i = 0; i < ARRAY_SIZE(start_code); i += 16) {
> +		int j, jmax;
> +		outw(i * 2, ioaddr + SM_PTR);
> +
> +		jmax = min_t(int, 16, ARRAY_SIZE(start_code) - i);
> +		for (j = 0; j < jmax; j++)
> +			outw(start_code[i + j],
> +			     ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16));
>  	}
>  
>  	/* Do we want promiscuous mode or multicast? */
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] eexpress: Read buffer overflow

[Jarek Poplawski]

Jarek Poplawski wrote, On 07/26/2009 10:16 PM:

> Roel Kluin wrote, On 07/25/2009 09:50 PM:
> 
>> start_code is 69 words, but the code always writes a multiple of 16 words,
>> so the last 11 words written are outside the array.
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> This was observed using Parfait http://research.sun.com/projects/parfait/
>>
>> diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
>> index 1686dca..e304abb 100644
>> --- a/drivers/net/eexpress.c
>> +++ b/drivers/net/eexpress.c
>> @@ -1474,15 +1474,14 @@ static void eexp_hw_init586(struct net_device *dev)
>>  	outw(0x0000, ioaddr + 0x800c);
>>  	outw(0x0000, ioaddr + 0x800e);
>>  
>> -	for (i = 0; i < (sizeof(start_code)); i+=32) {
>> -		int j;
>> -		outw(i, ioaddr + SM_PTR);
>> -		for (j = 0; j < 16; j+=2)
>> -			outw(start_code[(i+j)/2],
>> -			     ioaddr+0x4000+j);
>> -		for (j = 0; j < 16; j+=2)
>> -			outw(start_code[(i+j+16)/2],
>> -			     ioaddr+0x8000+j);
> 
> 
> (max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
> less than its size?


OOPS: (max) i = 128, (max) j = 14, (128+14+16)/2 = 79, so you are right!

Sorry,
Jarek P.

> 
>> +	for (i = 0; i < ARRAY_SIZE(start_code); i += 16) {
>> +		int j, jmax;
>> +		outw(i * 2, ioaddr + SM_PTR);
>> +
>> +		jmax = min_t(int, 16, ARRAY_SIZE(start_code) - i);
>> +		for (j = 0; j < jmax; j++)
>> +			outw(start_code[i + j],
>> +			     ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16));
>>  	}
>>  
>>  	/* Do we want promiscuous mode or multicast? */
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] eexpress: Read buffer overflow

[David Miller]

From: Roel Kluin <roel.kluin@gmail.com>
Date: Sat, 25 Jul 2009 21:50:19 +0200

> -	for (i = 0; i < (sizeof(start_code)); i+=32) {
> -		int j;
> -		outw(i, ioaddr + SM_PTR);
> -		for (j = 0; j < 16; j+=2)
> -			outw(start_code[(i+j)/2],
> -			     ioaddr+0x4000+j);
> -		for (j = 0; j < 16; j+=2)
> -			outw(start_code[(i+j+16)/2],
> -			     ioaddr+0x8000+j);
> +	for (i = 0; i < ARRAY_SIZE(start_code); i += 16) {
> +		int j, jmax;
> +		outw(i * 2, ioaddr + SM_PTR);
> +
> +		jmax = min_t(int, 16, ARRAY_SIZE(start_code) - i);
> +		for (j = 0; j < jmax; j++)
> +			outw(start_code[i + j],
> +			     ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16));

This new IO address expression:

	ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16)

DOES NOT match what the existing code does.  That 0x8000 - 16 seems
incorrect, the existing code always writes starting at 0x8000
in two byte increments, it does not start at 0x8000 - 16....

Oh nevermind, I see what you're doing.  Once j gets to 8, we have
to account for that in the IO address computation.

You've murdered this code, it's even more obfuscated now than it was
previously.  I'm not applying this, no way.  To a fix an out of
bounds array access you're going to change the loop iteration factors
and all of the sub-expressions within' 3 loops too?  Get real.

Just add the necessary limit tests, and nothing more, so it's
possible to actually understand your patch.  If it's more than
a 3 line patch, I'm not even going to review it.

Re: [PATCH] eexpress: Read buffer overflow

[Roel Kluin]

start_code is 69 words, but the code always writes a multiple of 16 words,
so the last 11 words written are outside the array.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Found with Parfait, http://research.sun.com/projects/parfait/

> You've murdered this code, it's even more obfuscated now than it was
> previously. 

Was it really that much worse? I though it was more clean. I did test
it to make sure that the semantics were the same.

> Just add the necessary limit tests, and nothing more, so it's
> possible to actually understand your patch.  If it's more than
> a 3 line patch, I'm not even going to review it.

3 lines it is, although scripts/checkpatch.pl doesn't like it.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Found with Parfait, http://research.sun.com/projects/parfait/

3 lines it is, but scripts/checkpatch.pl doesn't like it.

diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
index 1686dca..7b40014 100644
--- a/drivers/net/eexpress.c
+++ b/drivers/net/eexpress.c
@@ -1474,13 +1474,13 @@ static void eexp_hw_init586(struct net_device *dev)
 	outw(0x0000, ioaddr + 0x800c);
 	outw(0x0000, ioaddr + 0x800e);
 
-	for (i = 0; i < (sizeof(start_code)); i+=32) {
+	for (i = 0; i < ARRAY_SIZE(start_code); i+=32) {
 		int j;
 		outw(i, ioaddr + SM_PTR);
-		for (j = 0; j < 16; j+=2)
+		for (j = 0; j < 16 && (i+j)/2 < ARRAY_SIZE(start_code); j+=2)
 			outw(start_code[(i+j)/2],
 			     ioaddr+0x4000+j);
-		for (j = 0; j < 16; j+=2)
+		for (j = 0; j < 16 && (i+j+16)/2 < ARRAY_SIZE(start_code); j+=2)
 			outw(start_code[(i+j+16)/2],
 			     ioaddr+0x8000+j);
 	}

Re: [PATCH] eexpress: Read buffer overflow

[Jarek Poplawski]

On 29-07-2009 14:15, Roel Kluin wrote:
...
> 3 lines it is, but scripts/checkpatch.pl doesn't like it.
> 
> diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
> index 1686dca..7b40014 100644
> --- a/drivers/net/eexpress.c
> +++ b/drivers/net/eexpress.c
> @@ -1474,13 +1474,13 @@ static void eexp_hw_init586(struct net_device *dev)
>  	outw(0x0000, ioaddr + 0x800c);
>  	outw(0x0000, ioaddr + 0x800e);
>  
> -	for (i = 0; i < (sizeof(start_code)); i+=32) {
> +	for (i = 0; i < ARRAY_SIZE(start_code); i+=32) {
>  		int j;
>  		outw(i, ioaddr + SM_PTR);
> -		for (j = 0; j < 16; j+=2)
> +		for (j = 0; j < 16 && (i+j)/2 < ARRAY_SIZE(start_code); j+=2)
>  			outw(start_code[(i+j)/2],
>  			     ioaddr+0x4000+j);
> -		for (j = 0; j < 16; j+=2)
> +		for (j = 0; j < 16 && (i+j+16)/2 < ARRAY_SIZE(start_code); j+=2)
>  			outw(start_code[(i+j+16)/2],
>  			     ioaddr+0x8000+j);
>  	}

Now you seem to make my previous math working :-)

>> (max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
>> less than its size?

Jarek P.

Re: [PATCH] eexpress: Read buffer overflow

[Roel Kluin]

start_code is 69 words, but the code always writes a multiple of 16 words,
so the last 11 words written are outside the array.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
> Now you seem to make my previous math working :-)
> 
>>> (max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
>>> less than its size?
> 
> Jarek P.
> 

You're right, thanks for reviewing, this one should be correct.

diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
index 1686dca..1f016d6 100644
--- a/drivers/net/eexpress.c
+++ b/drivers/net/eexpress.c
@@ -1474,13 +1474,13 @@ static void eexp_hw_init586(struct net_device *dev)
 	outw(0x0000, ioaddr + 0x800c);
 	outw(0x0000, ioaddr + 0x800e);
 
-	for (i = 0; i < (sizeof(start_code)); i+=32) {
+	for (i = 0; i < ARRAY_SIZE(start_code) * 2; i+=32) {
 		int j;
 		outw(i, ioaddr + SM_PTR);
-		for (j = 0; j < 16; j+=2)
+		for (j = 0; j < 16 && (i+j)/2 < ARRAY_SIZE(start_code); j+=2)
 			outw(start_code[(i+j)/2],
 			     ioaddr+0x4000+j);
-		for (j = 0; j < 16; j+=2)
+		for (j = 0; j < 16 && (i+j+16)/2 < ARRAY_SIZE(start_code); j+=2)
 			outw(start_code[(i+j+16)/2],
 			     ioaddr+0x8000+j);
 	}

Re: [PATCH] eexpress: Read buffer overflow

[Jarek Poplawski]

On Wed, Jul 29, 2009 at 03:18:56PM +0200, Roel Kluin wrote:
> start_code is 69 words, but the code always writes a multiple of 16 words,
> so the last 11 words written are outside the array.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> > Now you seem to make my previous math working :-)
> > 
> >>> (max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
> >>> less than its size?
> > 
> > Jarek P.
> > 
> 
> You're right, thanks for reviewing, this one should be correct.

Looks OK to me.

Thanks,
Jarek P.

> 
> diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
> index 1686dca..1f016d6 100644
> --- a/drivers/net/eexpress.c
> +++ b/drivers/net/eexpress.c
> @@ -1474,13 +1474,13 @@ static void eexp_hw_init586(struct net_device *dev)
>  	outw(0x0000, ioaddr + 0x800c);
>  	outw(0x0000, ioaddr + 0x800e);
>  
> -	for (i = 0; i < (sizeof(start_code)); i+=32) {
> +	for (i = 0; i < ARRAY_SIZE(start_code) * 2; i+=32) {
>  		int j;
>  		outw(i, ioaddr + SM_PTR);
> -		for (j = 0; j < 16; j+=2)
> +		for (j = 0; j < 16 && (i+j)/2 < ARRAY_SIZE(start_code); j+=2)
>  			outw(start_code[(i+j)/2],
>  			     ioaddr+0x4000+j);
> -		for (j = 0; j < 16; j+=2)
> +		for (j = 0; j < 16 && (i+j+16)/2 < ARRAY_SIZE(start_code); j+=2)
>  			outw(start_code[(i+j+16)/2],
>  			     ioaddr+0x8000+j);
>  	}

Re: [PATCH] eexpress: Read buffer overflow

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 29 Jul 2009 22:05:17 +0200

> On Wed, Jul 29, 2009 at 03:18:56PM +0200, Roel Kluin wrote:
>> start_code is 69 words, but the code always writes a multiple of 16 words,
>> so the last 11 words written are outside the array.
>> 
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> > Now you seem to make my previous math working :-)
>> > 
>> >>> (max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
>> >>> less than its size?
>> > 
>> > Jarek P.
>> > 
>> 
>> You're right, thanks for reviewing, this one should be correct.
> 
> Looks OK to me.

Applied.