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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 10 Aug 2009 13:29:52 +0200
From:	Roel Kluin <roel.kluin@...il.com>
To:	David Miller <davem@...emloft.net>
CC:	khc@...waw.pl, netdev@...r.kernel.org, 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,
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?
> 
> 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++)
     {
--
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