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

[Paolo Perego]

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@xxxxxxx>
> > > > ---
> > > 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

Attachment: signature.asc
Description: PGP signature