Patchwork KS8851: Possible NULL dereferenced in ks8851_rx_pkts

login
register
mail settings
Submitter Abraham Arce
Date April 8, 2010, 7:03 a.m.
Message ID <z2ocb8016981004080003h7f5ca358r7a1a4106809909ce@mail.gmail.com>
Download mbox | patch
Permalink /patch/49701/
State Rejected
Delegated to: David Miller
Headers show

Comments

Abraham Arce - April 8, 2010, 7:03 a.m.
Hi,

These changes avoid a possible dereference in skb_reserve when skb is
NULL. I am increasing rx dropped packet count but not sure about how
to handle the dump of frames. Any advice is appreciated.


Best Regards
Abraham
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 13, 2010, 8:28 a.m.
From: Abraham Arce <abraham.arce.moreno@gmail.com>
Date: Thu, 8 Apr 2010 02:03:57 -0500

> Hi,
> 
> These changes avoid a possible dereference in skb_reserve when skb is
> NULL. I am increasing rx dropped packet count but not sure about how
> to handle the dump of frames. Any advice is appreciated.

This isn't sufficient to handle the NULL pointer.

At a minimum you're going to have to do something about
the fact that the chip has already been told to start
bringing the packet into the RX fifo.

If we just return without finishing up the hardware
operation, the chip is probably going to hang the next
time we come in here and tell it to start another RX
DMA operation without having completed the previous one.

The bug definitely needs to be fixed, however, but someone who knows
this hardware and has access to it for testing will need to implement
the fix.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Abraham Arce - May 5, 2010, 8:41 a.m.
David,

>> These changes avoid a possible dereference in skb_reserve when skb is
>> NULL. I am increasing rx dropped packet count but not sure about how
>> to handle the dump of frames. Any advice is appreciated.
>
> This isn't sufficient to handle the NULL pointer.
>
> At a minimum you're going to have to do something about
> the fact that the chip has already been told to start
> bringing the packet into the RX fifo.

Ok

>
> If we just return without finishing up the hardware
> operation, the chip is probably going to hang the next
> time we come in here and tell it to start another RX
> DMA operation without having completed the previous one.
>
> The bug definitely needs to be fixed, however, but someone who knows
> this hardware and has access to it for testing will need to implement
> the fix.
>

I'll give a try and test, have a hardware with me...

Thanks!
Abraham
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
index d6dc29b..a1aa757 100644
--- a/drivers/net/ks8851.c
+++ b/drivers/net/ks8851.c
@@ -534,8 +534,11 @@  static void ks8851_rx_pkts(struct ks8851_net *ks)

 		if (rxlen > 0) {
 			skb = netdev_alloc_skb(ks->netdev, rxlen + 2 + 8);
-			if (!skb) {
+			if (unlikely(!skb)) {
 				/* todo - dump frame and move on */
+				ks_dbg(ks, "No free memory, packet dropped\n");
+				ks->netdev->stats.rx_dropped++;
+				return;
 			}

 			/* two bytes to ensure ip is aligned, and four bytes