netdev - Re: [PATCH] lmc: Read outside array bounds

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <4A7C401B.7090301@gmail.com>
Date:	Fri, 07 Aug 2009 16:54:19 +0200
From:	Roel Kluin <roel.kluin@...il.com>
To:	"David S. Miller" <davem@...emloft.net>, khc@...waw.pl,
	netdev <netdev@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] 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 <roel.kluin@...il.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");
 }
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html