linux-kernel - Re: [PATCH] staging:gpib: Fix a dereference before null check issue

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: <b843f8a9-1562-458c-8f6a-c59b1037b756@stanley.mountain>
Date: Thu, 21 Nov 2024 10:37:30 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Kees Bakker <kees@...erbout.nl>
Cc: Paolo Perego <pperego@...e.de>, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org, Dave Penkler <dpenkler@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] staging:gpib: Fix a dereference before null check issue

On Wed, Nov 20, 2024 at 08:54:16PM +0100, Kees Bakker wrote:
> Op 20-11-2024 om 18:04 schreef Dan Carpenter:
> > On Wed, Nov 20, 2024 at 03:46:53PM +0100, Paolo Perego wrote:
> > > This commit fixes a dereference before null check issue discovered by
> > > Coverity (CID 1601566).
> > > 
> > > The check ad line 1450 suggests that a_priv can be NULL, however it has
> > > been derefenced before, in the interface_to_usbdev() call.
> > > 
> > > Signed-off-by: Paolo Perego <pperego@...e.de>
> > > ---
> > You need a Fixes tag.  But I'm pretty sure the correct fix is to remove the NULL
> > check.
> In the whole agilent_82357a.c module there is no consistency whether
> board->private_data needs to be checked for a NULL or not.
> 
> If Dan is correct then it is better to drop the NULL check, not only here
> but in a few more places as well. There are at least 10 functions were
> there is no NULL check for private_data.
> 
> Run this command and you'll see what I mean
> git grep -3 -e '->private_data' -- drivers/staging/gpib/agilent_82357a
> 

I had looked at similar issue in a different driver:
https://lore.kernel.org/all/2d99b7a6-f427-4d54-bde7-fb3df5e19e53@stanley.mountain/

Here the NULL check we are discussing is the same thing.  The private data is
allocated in attach() and freed in detach().  The detach has no need to check
for NULL because we can't detach something which isn't attached.

The other NULL checks are in agilent_82357a_driver_disconnect(),
agilent_82357a_driver_suspend() and agilent_82357a_driver_resume().  And there
the NULL checks are required because it could happen when the driver isn't
attached.

I also did a quick glance through to see if any of the functions which didn't
check for NULL should get a NULL check but they all seemed okay because either
the board was attached or the caller had a NULL check.

So I think we can just remove this one NULL check and everything else makes
sense.

regards,
dan carpenter