linux-kernel - Re: [PATCH] Fix a potential NULL pointer dereference in uli526x_interrupt() in drivers/net/tulip/uli526x.c

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: <alpine.LFD.0.999.0709041352450.13651@enigma.security.iitk.ac.in>
Date:	Tue, 4 Sep 2007 14:06:31 +0530 (IST)
From:	Satyam Sharma <satyam@...radead.org>
To:	Micah Gruber <micah.gruber@...il.com>
cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux Netdev Mailing List <netdev@...r.kernel.org>,
	jgarzik@...ox.com
Subject: Re: [PATCH] Fix a potential NULL pointer dereference in uli526x_interrupt()
 in drivers/net/tulip/uli526x.c

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 <micah.gruber@...il.com>
> ---
> 
> --- 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)"
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/