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] [day] [month] [year] [list]

Message-ID: <47c36187-7315-4a32-802c-4909599e36f7@stanley.mountain>
Date: Thu, 21 Nov 2024 17:17:10 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Paolo Perego <pperego@...e.de>
Cc: Kees Bakker <kees@...erbout.nl>, 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 Thu, Nov 21, 2024 at 03:10:02PM +0100, Paolo Perego wrote:
> On Thu, Nov 21, 2024 at 10:37:30AM GMT, Dan Carpenter wrote:
> > 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.
> > 
> 
> Hi all and thanks for the fruitful discussion. 
> 
> > So I think we can just remove this one NULL check and everything else makes
> > sense.
> Please, apologise if I'm too newbie here to understand next step on my
> own. Am I asked to do something, to submit a V2 with the correct Tag or
> the patch is good as is?
> 
> ( No pressure to be accepted, it's just my willing to understand and go
> into the process :-) )
> 

Please send a v2 which deletes the NULL check.

regards,
dan carpenter