Skip to contentSkip to conversation

eexpress: Read buffer overflow

PATCH Applied Roel Kluin 3 participants 9 messages Jul 25, 2009 – Jul 30, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 eexpress: Read buffer overflow

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 <[email protected]>

---
This was observed using Parfait http://research.sun.com/projects/parfait/

drivers/net/eexpress.c +8 -9
index 1686dca..e304abb 100644
@@ -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? */
#1 lore raw

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

Quoted 14 lines

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 <[email protected]>

---
This was observed using Parfait http://research.sun.com/projects/parfait/

drivers/net/eexpress.c +0 -0
index 1686dca..e304abb 100644
@@ -1474,15 +1474,14 @@ static void eexp_hw_init586(struct net_device *dev)
 	outw(0x0000, ioaddr + 0x800c);
 	outw(0x0000, ioaddr + 0x800e);

drivers/net/eexpress.c static void eexp_hw_init586(struct net_device *dev) · line 1485

Quoted 10 lines
drivers/net/eexpress.c +0 -9
 
-	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.

Quoted 16 lines
drivers/net/eexpress.c +8 -1
+	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw

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

Quoted 17 lines

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

Quoted 15 lines

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 <[email protected]>

---
This was observed using Parfait http://research.sun.com/projects/parfait/

drivers/net/eexpress.c +0 -0
index 1686dca..e304abb 100644
@@ -1474,15 +1474,14 @@ static void eexp_hw_init586(struct net_device *dev)
 	outw(0x0000, ioaddr + 0x800c);
 	outw(0x0000, ioaddr + 0x800e);
Quoted 13 lines

drivers/net/eexpress.c static void eexp_hw_init586(struct net_device *dev) · line 1485

Quoted 9 lines
drivers/net/eexpress.c +0 -9
-	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.

Quoted 24 lines
Quoted 13 lines
drivers/net/eexpress.c +8 -1
+	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? */
--
Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#3 lore raw
In reply to Roel Kluin

From: Roel Kluin <[email protected]>
Date: Sat, 25 Jul 2009 21:50:19 +0200

Quoted 7 lines
drivers/net/eexpress.c +0 -7
-	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)

drivers/net/eexpress.c static void eexp_hw_init586(struct net_device *dev) · line 1484

Quoted 10 lines
drivers/net/eexpress.c +8 -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.

author
#4 lore raw

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 <[email protected]>

---
Found with Parfait, http://research.sun.com/projects/parfait/

Quoted 2 lines

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.

Quoted 3 lines

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 <[email protected]>

---
Found with Parfait, http://research.sun.com/projects/parfait/

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

drivers/net/eexpress.c +3 -3
index 1686dca..7b40014 100644
@@ -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);
 	}
#5 lore raw

On 29-07-2009 14:15, Roel Kluin wrote:
...

Quoted 13 lines

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

drivers/net/eexpress.c +1 -1
index 1686dca..7b40014 100644
@@ -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;

drivers/net/eexpress.c static void eexp_hw_init586(struct net_device *dev) · line 1486

Quoted 10 lines
drivers/net/eexpress.c +2 -2
 		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 :-)

Quoted 2 lines
Quoted 2 lines

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

Jarek P.

author
#6 lore raw

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 <[email protected]>

---

Quoted 7 lines

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

Quoted 2 lines
Quoted 2 lines

(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.

drivers/net/eexpress.c +3 -3
index 1686dca..1f016d6 100644
@@ -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);
 	}
#7 lore raw
Quoted 7 lines · 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 <[email protected]>

---

Quoted 2 lines

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

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

(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.

Quoted 22 lines
drivers/net/eexpress.c +3 -3
index 1686dca..1f016d6 100644
@@ -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);
 	}
maintainer Applied
#8 lore raw

From: Jarek Poplawski <[email protected]>
Date: Wed, 29 Jul 2009 22:05:17 +0200

Quoted 11 lines · Roel Kluin
Quoted 10 lines

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 <[email protected]>

---

Quoted 5 lines

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

Quoted 2 lines
Quoted 2 lines

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

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Jarek P.

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

Looks OK to me.

Applied.