KS8851: Possible NULL dereferenced in ks8851_rx_pkts

KS8851: Possible NULL dereferenced in ks8851_rx_pkts

[Abraham Arce]

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.

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

Best Regards
Abraham

Re: KS8851: Possible NULL dereferenced in ks8851_rx_pkts

[David Miller]

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.

Re: KS8851: Possible NULL dereferenced in ks8851_rx_pkts

[Abraham Arce]

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