Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753904AbXIDIh2 (ORCPT ); Tue, 4 Sep 2007 04:37:28 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752404AbXIDIhN (ORCPT ); Tue, 4 Sep 2007 04:37:13 -0400 Received: from pentafluge.infradead.org ([213.146.154.40]:45464 "EHLO pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752607AbXIDIhM (ORCPT ); Tue, 4 Sep 2007 04:37:12 -0400 Date: Tue, 4 Sep 2007 14:20:21 +0530 (IST) From: Satyam Sharma X-X-Sender: satyam@enigma.security.iitk.ac.in To: Micah Gruber cc: Linux Kernel Mailing List , Linux Netdev Mailing List , jgarzik@pobox.com Subject: Re: [PATCH] Fix a potential NULL pointer dereference in uli526x_interrupt() in drivers/net/tulip/uli526x.c In-Reply-To: Message-ID: References: <46DD121A.2010404@gmail.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1944 Lines: 62 On Tue, 4 Sep 2007, Satyam Sharma wrote: > Hi Micah, > > > On Tue, 4 Sep 2007, Micah Gruber wrote: > > > 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 > > --- > > > > --- a/drivers/net/tulip/uli526x.c > > +++ b/drivers/net/tulip/uli526x.c > > @@ -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); > > > [The patch is reversed.] > > But it is also complete -- you've left out the "netdev_priv(dev)" ^^^^^^^^ I meant: incomplete > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/