Skip to contentSkip to conversation

lmc: Read outside array bounds

PATCH Applied Roel Kluin 2 participants 6 messages Jul 25, 2009 – Aug 13, 2009 1 message not in the archive

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

Roel Kluin

author
#0 lore raw

PATCH 1/1 lmc: Read outside array bounds

if dev_alloc_skb() fails on the first iteration of the allocation loop, and we break out of the loop, then we end up writing before the start of the array.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/net/wan/lmc/lmc_main.c +0 -0
index 45b1822..ac8d5b2 100644
@@ -1865,7 +1865,7 @@ static void lmc_softreset (lmc_softc_t * const sc) /*fold00*/
             if(skb == NULL){
                 printk(KERN_WARNING "%s: Failed to allocate receiver ring, will try again

", sc->name);
                 sc->failed_ring = 1;
-                break;
+                return;
             }
             else{
                 sc->lmc_rxq[i] = skb;
maintainer
#1 lore raw

From: Roel Kluin <[email protected]>
Date: Sun, 26 Jul 2009 00:18:17 +0200

Quoted 4 lines

if dev_alloc_skb() fails on the first iteration of the allocation loop, and we break out of the loop, then we end up writing before the start of the array.

Signed-off-by Roel Kluin <[email protected]>

...

Review

Quoted 3 lines
                 sc->failed_ring = 1;
(quoted diff) +1 -1
-                break;
+                return;

This won't work.

First of all, if we allocated at least one buffer we should mark the last one in the code right after this loop.

Second of all, we should purge the TX skbs in the next loop even if we could not allocate even one RX buffer.

The thing to do is probably to guard the set of "[i-1]" RX ring accesses with a "if (i != 0)" check.

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#3 lore raw

If dev_alloc_skb() fails on the first iteration of the allocation loop, and we break out of the loop, then we end up writing before the start of the array.

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 8 lines

First of all, if we allocated at least one buffer we should mark the last one in the code right after this loop.

Second of all, we should purge the TX skbs in the next loop even if we could not allocate even one RX buffer.

The thing to do is probably to guard the set of "[i-1]" RX ring accesses with a "if (i != 0)" check.

Forgot a bit about this one, but I hope this is what you meant?

drivers/net/wan/lmc/lmc_main.c +16 -13
index 45b1822..b26fabb 100644
@@ -1838,7 +1838,7 @@ void lmc_mii_writereg (lmc_softc_t * const sc, unsigned devaddr, unsigned regno,
 
 static void lmc_softreset (lmc_softc_t * const sc) /*fold00*/
 {
-    int i;
+    int i, j;
 
     lmc_trace(sc->lmc_device, "lmc_softreset in");
 

@@ -1897,24 +1897,27 @@ static void lmc_softreset (lmc_softc_t * const sc) /*fold00*/
     /*
      * Sets end of ring
      */
-    sc->lmc_rxring[i - 1].length |= 0x02000000; /* Set end of buffers flag */
-    sc->lmc_rxring[i - 1].buffer2 = virt_to_bus (&sc->lmc_rxring[0]); /* Point back to the start */
-    LMC_CSR_WRITE (sc, csr_rxlist, virt_to_bus (sc->lmc_rxring)); /* write base address */
+    if (i > 0) {
+        sc->lmc_rxring[i - 1].length |= 0x02000000; /* Set end of buffers flag */
+        sc->lmc_rxring[i - 1].buffer2 = virt_to_bus(&sc->lmc_rxring[0]); /* Point back to the start */
+        LMC_CSR_WRITE(sc, csr_rxlist, virt_to_bus(sc->lmc_rxring)); /* write base address */
+    }
 
 
     /* Initialize the transmit rings and buffers */
-    for (i = 0; i < LMC_TXDESCS; i++)
-    {
-        if (sc->lmc_txq[i] != NULL){		/* have buffer */
-            dev_kfree_skb(sc->lmc_txq[i]);	/* free it */
+    for (j = 0; j < i; j++) {
+        if (sc->lmc_txq[j] != NULL) {		/* have buffer */
+            dev_kfree_skb(sc->lmc_txq[j]);	/* free it */
 	    sc->lmc_device->stats.tx_dropped++;	/* We just dropped a packet */
         }
-        sc->lmc_txq[i] = NULL;
-        sc->lmc_txring[i].status = 0x00000000;
-        sc->lmc_txring[i].buffer2 = virt_to_bus (&sc->lmc_txring[i + 1]);
+        sc->lmc_txq[j] = NULL;
+        sc->lmc_txring[j].status = 0x00000000;
+        sc->lmc_txring[j].buffer2 = virt_to_bus(&sc->lmc_txring[j + 1]);
+    }
+    if (j > 0) {
+        sc->lmc_txring[j - 1].buffer2 = virt_to_bus(&sc->lmc_txring[0]);
+        LMC_CSR_WRITE(sc, csr_txlist, virt_to_bus(sc->lmc_txring));
     }
-    sc->lmc_txring[i - 1].buffer2 = virt_to_bus (&sc->lmc_txring[0]);
-    LMC_CSR_WRITE (sc, csr_txlist, virt_to_bus (sc->lmc_txring));
 
     lmc_trace(sc->lmc_device, "lmc_softreset out");
 }
maintainer
#4 lore raw

From: Roel Kluin <[email protected]>
Date: Fri, 07 Aug 2009 16:54:19 +0200

Quoted 8 lines

If dev_alloc_skb() fails on the first iteration of the allocation loop, and we break out of the loop, then we end up writing before the start of the array.

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 3 lines

First of all, if we allocated at least one buffer we should mark the last one in the code right after this loop.

Quoted 7 lines
Quoted 5 lines

Second of all, we should purge the TX skbs in the next loop even if we could not allocate even one RX buffer.

The thing to do is probably to guard the set of "[i-1]" RX ring accesses with a "if (i != 0)" check.

Forgot a bit about this one, but I hope this is what you meant?

It's not correct to limit the TX ring loop by how many RX ring buffers we've been able to successfully allocate, that doesn't make any sense.

I'm talking about the second loop which you unexplainably changed to "for (j = 0; j < i; ..."

I'm not applying this.

author
#5 lore raw

If dev_alloc_skb() fails on the first iteration of the allocation loop, then we end up writing before the start of the array.

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

First of all, if we allocated at least one buffer we should mark the last one in the code right after this loop.

Second of all, we should purge the TX skbs in the next loop even if we could not allocate even one RX buffer.

The thing to do is probably to guard the set of "[i-1]" RX ring accesses with a "if (i != 0)" check.

Quoted 8 lines
Quoted 1 line

Forgot a bit about this one, but I hope this is what you meant?

It's not correct to limit the TX ring loop by how many RX ring buffers we've been able to successfully allocate, that doesn't make any sense.

I'm talking about the second loop which you unexplainably changed to "for (j = 0; j < i; ..."

Ok, I apparently completely misunderstood. One remaining question is whether the base address should still be written if i is 0?

drivers/net/wan/lmc/lmc_main.c +4 -3
index 45b1822..8b61660 100644
@@ -1897,11 +1897,12 @@ static void lmc_softreset (lmc_softc_t * const sc) /*fold00*/
     /*
      * Sets end of ring
      */
-    sc->lmc_rxring[i - 1].length |= 0x02000000; /* Set end of buffers flag */
-    sc->lmc_rxring[i - 1].buffer2 = virt_to_bus (&sc->lmc_rxring[0]); /* Point back to the start */
+    if (i != 0) {
+        sc->lmc_rxring[i - 1].length |= 0x02000000; /* Set end of buffers flag */
+        sc->lmc_rxring[i - 1].buffer2 = virt_to_bus(&sc->lmc_rxring[0]); /* Point back to the start */
+    }
     LMC_CSR_WRITE (sc, csr_rxlist, virt_to_bus (sc->lmc_rxring)); /* write base address */
 
-
     /* Initialize the transmit rings and buffers */
     for (i = 0; i < LMC_TXDESCS; i++)
     {
maintainer Applied
#6 lore raw

From: Roel Kluin <[email protected]>
Date: Mon, 10 Aug 2009 13:29:52 +0200

Quoted 4 lines

If dev_alloc_skb() fails on the first iteration of the allocation loop, then we end up writing before the start of the array.

Signed-off-by Roel Kluin <[email protected]>

Applied to net-next-2.6