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: <wgcv5wesq3q3xank6elouewntisernnw4agfziqekkeirftx6i@ajucs5yqsvus>
Date: Thu, 21 Nov 2024 15:10:02 +0100
From: Paolo Perego <pperego@...e.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
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 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 :-) )

Thanks
Paolo
-- 
(*_  Paolo Perego                           @thesp0nge
//\  Software security engineer               suse.com
V_/_ 0A1A 2003 9AE0 B09C 51A4 7ACD FC0D CEA6 0806 294B

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)