staging: gpib: Fix NULL deref and code cleanup

When the detach function is called after a failed attach the usb_dev initialization can cause a NULL pointer dereference. This happens when the usb device is not found in the attach procedure.

Remove the usb_dev variable and initialization and change the dev in the dev_info message from the usb_dev to the gpib_dev.

Fixes fbae7090f30c ("staging: gpib: Update messaging and usb_device refs in agilent_usb")

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/staging/gpib/agilent_82357a/agilent_82357a.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/staging/gpib/agilent_82357a/agilent_82357a.c +0 -2

index 34d85a1bdb37..2aaccebc3c7b 100644
@@ -1442,12 +1442,10 @@ static int agilent_82357a_go_idle(gpib_board_t *board)
 static void agilent_82357a_detach(gpib_board_t *board)
 {
 	struct agilent_82357a_priv *a_priv;
-	struct usb_device *usb_dev;
 
 	mutex_lock(&agilent_82357a_hotplug_lock);
 
 	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);

Quoted 32 lines · Dave Penkler

When the detach function is called after a failed attach the usb_dev initialization can cause a NULL pointer dereference. This happens when the usb device is not found in the attach procedure.

Remove the usb_dev variable and initialization and change the dev in the dev_info message from the usb_dev to the gpib_dev.

Fixes fbae7090f30c ("staging: gpib: Update messaging and usb_device refs in agilent_usb")

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/staging/gpib/agilent_82357a/agilent_82357a.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/staging/gpib/agilent_82357a/agilent_82357a.c +0 -2

index 34d85a1bdb37..2aaccebc3c7b 100644
@@ -1442,12 +1442,10 @@ static int agilent_82357a_go_idle(gpib_board_t *board)
 static void agilent_82357a_detach(gpib_board_t *board)
 {
 	struct agilent_82357a_priv *a_priv;
-	struct usb_device *usb_dev;
 
 	mutex_lock(&agilent_82357a_hotplug_lock);
 
 	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);

drivers/staging/gpib/agilent_82357a/agilent_82357a.c static void agilent_82357a_detach(gpib_board_t *board) · line 1460

Quoted 5 lines

drivers/staging/gpib/agilent_82357a/agilent_82357a.c +1 -1

@@ -1459,7 +1457,7 @@ static void agilent_82357a_detach(gpib_board_t *board)
 		agilent_82357a_cleanup_urbs(a_priv);
 		agilent_82357a_free_private(a_priv);
 	}
-	dev_info(&usb_dev->dev, "%s: detached\n", __func__);
+	dev_info(board->gpib_dev, "%s: detached\n", __func__);

In the future, these dev_info() lines all need to go away as when drivers work properly, they should be quiet. I'll take this fix for now as it's needed, just something to plan on for the future cleanups here.

thanks,

greg k-h

When the detach function is called after a failed attach the usb_dev initialization can cause a NULL pointer dereference. This happens when the usb device is not found in the attach procedure.

Remove useless #ifdef RESET_USB_CONFIG code.

Change kalloc / memset to kzalloc

The attach function was not freeing the private data on error returns. Separate the releasing of urbs and private data and add a common error exit for attach failure.

Set the board private data pointer to NULL after freeing the private data.

Reduce console spam by emitting only one attach message.

Change last pr_err in attach to dev_err

This patch does too many things... It should be split up. People complain about this requirement a lot, but eventually it will become instinctive. I use git citool so I can highlight and click to add lines to a commit. In this code there were some dev_info() changes mixed into the unwind code in ->attach() that were hard to separate out into their own commit but it wasn't too complicated.

Quoted 2 lines · Dave Penkler

Remove useless #ifdef RESET_USB_CONFIG code.

patch 1.

Quoted 2 lines

Change kalloc / memset to kzalloc

patch 2.

Quoted 6 lines

The attach function was not freeing the private data on error returns. Separate the releasing of urbs and private data and add a common error exit for attach failure.

Set the board private data pointer to NULL after freeing the private data.

By setting the private data, this patch actually does fix the double free that I mentioned earlier. It changes the ->detach into a no-op if ->attach fails. Needs a Fixes tag. ;)

But I still hope my blog will convince you that the error handling can be re-written in a better way. It shouldn't matter if ->private_data is NULL or non-NULL because the caller should only have to handle success or failure. The caller shouldn't have to handle a dozen different failure modes:

1) Failure but the ->private_data is NULL
2) Failure but the foo->frob pointer is an error pointer
3) Failure but the foo->frob pointer needs to be freed.
4) Failure but the foo->frob pointer contains other pointers which need to be freed.
5) ...

It should just be

1) Success: Everything is allocated
2) Failure: Everything is cleaned up and any accesses are probably a use after free.

Quoted 5 lines

Reduce console spam by emitting only one attach message.

Change last pr_err in attach to dev_err

These last two can probably be combined into one patch?

Quoted 3 lines

drivers/staging/gpib/agilent_82357a/agilent_82357a.c +0 -0

@@ -1388,11 +1367,19 @@ static int agilent_82357a_attach(gpib_board_t *board, const gpib_board_config_t
 	retval = agilent_82357a_init(board);
 

drivers/staging/gpib/agilent_82357a/agilent_82357a.c static int agilent_82357a_attach(gpib_board_t *board, const gpib_board_config_t · line 1373

Quoted 7 lines

drivers/staging/gpib/agilent_82357a/agilent_82357a.c +3 -2

 	if (retval < 0)	{
-		mutex_unlock(&agilent_82357a_hotplug_lock);
-		return retval;
+		agilent_82357a_cleanup_urbs(a_priv);
+		agilent_82357a_release_urbs(a_priv);
+		goto attach_fail;
 	}

In my blog talk about how every allocation function should have a matching free() function. These two functions match agilent_82357a_setup_urbs() so we should have a single function to release the urbs.

regards,
dan carpenter