Fix a potential NULL pointer dereference in uli526x_interrupt() in drivers/net/tulip/uli526x.c

PATCH In review Micah Gruber 2 participants 3 messages Sep 4, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Micah Gruber

author

2007-09-04 08:05 UTC #0 lore raw

PATCH 1/1 Fix a potential NULL pointer dereference in uli526x_interrupt() in drivers/net/tulip/uli526x.c

This patch fixes a potential null dereference bug where we dereference dev before a null check. This patch simply moves the dereferencing after the null check.

Signed-off-by Micah Gruber <[email protected]>

---

drivers/net/tulip/uli526x.c +1 -3

@@ -663,7 +663,7 @@
 {
 	struct net_device *dev = dev_id;
 	struct uli526x_board_info *db = netdev_priv(dev);
-	unsigned long ioaddr;
+	unsigned long ioaddr = dev->base_addr;
 	unsigned long flags;
 
 	if (!dev) {

@@ -671,8 +671,6 @@
 		return IRQ_NONE;
 	}
 
-	ioaddr = dev->base_addr;
-
 	spin_lock_irqsave(&db->lock, flags);
 	outl(0, ioaddr + DCR7);

But it is also complete -- you've left out the "netdev_priv(dev)" statement _just_ before the dereference that you've pushed down. Coverity wasn't smart enough to tell, but that's actually a dev->priv, so we'll _still_ be dereferencing dev before checking it for NULL below. And lastly, the patch isn't quite correct. It is the (!dev) check that is redundant and must be removed instead :-)

I bet more such pointless checks exist all over. It makes more sense to remove them than unnecessarily try to solve "potential" NULL dereferences, that we (naturally) never saw earlier, because they're impossible. ISTR pointing out two similar patches a month or so back when Jeff pushed net driver patches upstream, IIRC ... ]

Satyam

Satyam Sharma

2007-09-04 08:37 UTC #2 lore raw

Quoted 35 lines · Satyam Sharma

Hi Micah,
Quoted 19 lines · Micah Gruber
This patch fixes a potential null dereference bug where we dereference dev before a null check. This patch simply moves the dereferencing after the null check.
Signed-off-by Micah Gruber <[email protected]>
---
drivers/net/tulip/uli526x.c +1 -1
@@ -663,7 +663,7 @@
 {
 	struct net_device *dev = dev_id;
 	struct uli526x_board_info *db = netdev_priv(dev);
-	unsigned long ioaddr;
+	unsigned long ioaddr = dev->base_addr;
 	unsigned long flags;
 
 	if (!dev) {
drivers/net/tulip/uli526x.c line 675
Quoted 7 lines
drivers/net/tulip/uli526x.c +0 -2
@@ -671,8 +671,6 @@
 		return IRQ_NONE;
 	}
 
-	ioaddr = dev->base_addr;
-
 	spin_lock_irqsave(&db->lock, flags);
 	outl(0, ioaddr + DCR7);
[The patch is reversed.]

Quoted 1 line

But it is also complete -- you've left out the "netdev_priv(dev)"

                 ^^^^^^^^

I meant: incomplete

Quoted 14 lines

statement _just_ before the dereference that you've pushed down. Coverity wasn't smart enough to tell, but that's actually a dev->priv, so we'll _still_ be dereferencing dev before checking it for NULL below. And lastly, the patch isn't quite correct. It is the (!dev) check that is redundant and must be removed instead :-)
I bet more such pointless checks exist all over. It makes more sense to remove them than unnecessarily try to solve "potential" NULL dereferences, that we (naturally) never saw earlier, because they're impossible. ISTR pointing out two similar patches a month or so back when Jeff pushed net driver patches upstream, IIRC ... ]
Satyam