netdev - Re: [PATCH] korina: Read buffer overflow

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]

Message-ID: <25e057c00908080614q6d7efa3x46789e51b0b544b3@mail.gmail.com>
Date:	Sat, 8 Aug 2009 15:14:57 +0200
From:	roel kluin <roel.kluin@...il.com>
To:	Roel Kluin <roel.kluin@...il.com>, netdev@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	"David S. Miller" <davem@...emloft.net>, florian@...nwrt.org
Subject: Re: [PATCH] korina: Read buffer overflow

>> If the loop breaks with an i of 0, then we read lp->rd_ring[-1].

>> @@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev)
>>       for (i = 0; i < KORINA_NUM_RDS; i++) {
>>               skb = dev_alloc_skb(KORINA_RBSIZE + 2);
>>               if (!skb)
>> -                     break;
>> +                     goto err_free;
>
> This implies that all KORINA_NUM_TDS receive descriptors need to be
> available for the driver to work.

> Also I guess there should be some error handling, as the driver probably
> wont be useful without a single receive descriptor. What do you think
> about the following:

A few comments:

> | diff --git a/drivers/net/korina.c b/drivers/net/korina.c
> | index a2701f5..d1c5276 100644
> | --- a/drivers/net/korina.c
> | +++ b/drivers/net/korina.c

> | @@ -772,6 +772,13 @@ static void korina_alloc_ring(struct net_device *dev)
> |               lp->rd_ring[i].ca = CPHYSADDR(skb->data);
> |               lp->rd_ring[i].link = CPHYSADDR(&lp->rd_ring[i+1]);
> |       }
> | +     if (!i) {
> | +             printk(KERN_ERR DRV_NAME "%s: could not allocate a single"
> | +                             " receive descriptor\n", dev->name)

            printk(KERN_ERR "%s: could not allocate a single receive
descriptor\n",
                            dev->name);

Don't interrupt strings, they are an exception in the 80 character
limit (since they are
easier to grep that way). Also I think the DRV_NAME should be omitted, Doesn't
`DRV_NAME "%s:...\n", dev->name)' result in "korinakorina:..."? It
occurs at more
places in the file. Also a semicolon was missing.

> | +             return 1;

I think -ENOMEM is more appropriate.

> | +     }
> | +     printk(KERN_DEBUG DRV_NAME "%s: allocated %d receive descriptors\n",
> | +                     dev->name, i);

same `DRV_NAME "%s:...\n", dev->name)' issue.

> | @@ -824,7 +833,8 @@ static int korina_init(struct net_device *dev)
> |       writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc);
> |
> |       /* Allocate rings */
> | -     korina_alloc_ring(dev);
> | +     if (korina_alloc_ring(dev))
> | +             return 1;

-ENOMEM

> |
> |       writel(0, &lp->rx_dma_regs->dmas);
> |       /* Start Rx DMA */
>
> Greetings, Phil

Otherwise looks fine to me.

Thanks,

Roel
--
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