[PATCH] lmc: Read outside array bounds

[PATCH] lmc: Read outside array bounds

[Roel Kluin]

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 <roel.kluin@gmail.com>
---
diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
index 45b1822..ac8d5b2 100644
--- a/drivers/net/wan/lmc/lmc_main.c
+++ b/drivers/net/wan/lmc/lmc_main.c
@@ -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;

Re: [PATCH] lmc: Read outside array bounds

[David Miller]

From: Roel Kluin <roel.kluin@gmail.com>
Date: Sun, 26 Jul 2009 00:18:17 +0200

> 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 <roel.kluin@gmail.com>
...
>                  sc->failed_ring = 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.

Re: [PATCH] lmc: Read outside array bounds

[Roel Kluin]

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 <roel.kluin@gmail.com>
---
> 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?

diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
index 45b1822..b26fabb 100644
--- a/drivers/net/wan/lmc/lmc_main.c
+++ b/drivers/net/wan/lmc/lmc_main.c
@@ -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");
 }

Re: [PATCH] lmc: Read outside array bounds

[David Miller]

From: Roel Kluin <roel.kluin@gmail.com>
Date: Fri, 07 Aug 2009 16:54:19 +0200

> 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 <roel.kluin@gmail.com>
> ---
>> 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?

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.

Re: [PATCH] lmc: Read outside array bounds

[Roel Kluin]

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 <roel.kluin@gmail.com>
---
>>> 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?
> 
> 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?

diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
index 45b1822..8b61660 100644
--- a/drivers/net/wan/lmc/lmc_main.c
+++ b/drivers/net/wan/lmc/lmc_main.c
@@ -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++)
     {

Re: [PATCH] lmc: Read outside array bounds

[David Miller]

From: Roel Kluin <roel.kluin@gmail.com>
Date: Mon, 10 Aug 2009 13:29:52 +0200

> 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 <roel.kluin@gmail.com>

Applied to net-next-2.6