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

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

[Paolo Perego]

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@suse.de>
---
 drivers/staging/gpib/agilent_82357a/agilent_82357a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gpib/agilent_82357a/agilent_82357a.c b/drivers/staging/gpib/agilent_82357a/agilent_82357a.c
index bf05fb4a736b..604e13c32dfb 100644
--- a/drivers/staging/gpib/agilent_82357a/agilent_82357a.c
+++ b/drivers/staging/gpib/agilent_82357a/agilent_82357a.c
@@ -1446,8 +1446,8 @@ static void agilent_82357a_detach(gpib_board_t *board)
 	mutex_lock(&agilent_82357a_hotplug_lock);
 
 	a_priv = board->private_data;
-	usb_dev = interface_to_usbdev(a_priv->bus_interface);
 	if (a_priv) {
+		usb_dev = interface_to_usbdev(a_priv->bus_interface);
 		if (a_priv->bus_interface) {
 			agilent_82357a_go_idle(board);
 			usb_set_intfdata(a_priv->bus_interface, NULL);
-- 
2.47.0

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

[Markus Elfring]

…
> The check ad line 1450 suggests that a_priv can be NULL, however it has

            at?


> been derefenced before, in the interface_to_usbdev() call.
…

       dereferenced?


See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n94

Regards,
Markus

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

[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@suse.de>
> ---

You need a Fixes tag.  But I'm pretty sure the correct fix is to remove the NULL
check.

regards,
dan carpenter

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

[Kees Bakker]

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@suse.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

>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

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@suse.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

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

[Paolo Perego]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

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@suse.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dan Carpenter]

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@suse.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