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

[PATCH v2] 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 at line 1450 suggests that a_priv can be NULL, however it has
been dereferenced before, in the interface_to_usbdev() call.

After a discussion, the NULL check is useless because private data is allocated
in attach routine and freed in detach routine, so in detach routine the pointer
has to be not NULL for sure.

Signed-off-by: Paolo Perego <pperego@suse.de>
Fixes: Removes a useless NULL check

---
v2: 
* After a discussion, we agreed the NULL check is useless because private data
  is freed in detach routine and allocated while attaching. For such a reason,
  the pointer has to be not NULL while detaching the device

 .../gpib/agilent_82357a/agilent_82357a.c      | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/gpib/agilent_82357a/agilent_82357a.c b/drivers/staging/gpib/agilent_82357a/agilent_82357a.c
index bf05fb4a736b..e940429dea19 100644
--- a/drivers/staging/gpib/agilent_82357a/agilent_82357a.c
+++ b/drivers/staging/gpib/agilent_82357a/agilent_82357a.c
@@ -1447,17 +1447,17 @@ static void agilent_82357a_detach(gpib_board_t *board)
 
 	a_priv = board->private_data;
 	usb_dev = interface_to_usbdev(a_priv->bus_interface);
-	if (a_priv) {
-		if (a_priv->bus_interface) {
-			agilent_82357a_go_idle(board);
-			usb_set_intfdata(a_priv->bus_interface, NULL);
-		}
-		mutex_lock(&a_priv->control_alloc_lock);
-		mutex_lock(&a_priv->bulk_alloc_lock);
-		mutex_lock(&a_priv->interrupt_alloc_lock);
-		agilent_82357a_cleanup_urbs(a_priv);
-		agilent_82357a_free_private(a_priv);
+
+	if (a_priv->bus_interface) {
+		agilent_82357a_go_idle(board);
+		usb_set_intfdata(a_priv->bus_interface, NULL);
 	}
+	
+	mutex_lock(&a_priv->control_alloc_lock);
+	mutex_lock(&a_priv->bulk_alloc_lock);
+	mutex_lock(&a_priv->interrupt_alloc_lock);
+	agilent_82357a_cleanup_urbs(a_priv);
+	agilent_82357a_free_private(a_priv);
 	dev_info(&usb_dev->dev, "%s: detached\n", __func__);
 	mutex_unlock(&agilent_82357a_hotplug_lock);
 }
-- 
2.47.0

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

[Dan Carpenter]

On Thu, Nov 21, 2024 at 04:22:21PM +0100, Paolo Perego wrote:
> This commit fixes a dereference before null check issue discovered by 
> Coverity (CID 1601566).
> 
> The check at line 1450 suggests that a_priv can be NULL, however it has
> been dereferenced before, in the interface_to_usbdev() call.
> 
> After a discussion, the NULL check is useless because private data is allocated
> in attach routine and freed in detach routine, so in detach routine the pointer
> has to be not NULL for sure.
> 
> Signed-off-by: Paolo Perego <pperego@suse.de>
> Fixes: Removes a useless NULL check

This isn't how the Fixes tag works.  It's supposed to refer to the patch which
introduces the static checker warning.
Fixes: fbae7090f30c ("staging: gpib: Update messaging and usb_device refs in agilent_usb")

The patch has a couple checkpatch issues as well.  Please, fix those up and
resend.

regards,
dan carpenter