Skip to contentSkip to conversation

Fix a potential NULL pointer dereference in usbat_check_status() in drivers/usb/storage/shuttle_usbat.c

PATCH In review Micah Gruber 4 participants 7 messages Sep 4, 2007 – Sep 10, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix a potential NULL pointer dereference in usbat_check_status() in drivers/usb/storage/shuttle_usbat.c

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

---

drivers/usb/storage/shuttle_usbat.c +3 -1
@@ -187,12 +187,14 @@
  */
 static int usbat_check_status(struct us_data *us)
 {
-	unsigned char *reply = us->iobuf;
+	unsigned char *reply;
 	int rc;
 
 	if (!us)
 		return USB_STOR_TRANSPORT_ERROR;
 
+	reply = us->iobuf;
+
 	rc = usbat_get_status(us, reply);
 	if (rc != USB_STOR_XFER_GOOD)
 		return USB_STOR_TRANSPORT_FAILED;
#1 lore raw
Quoted 3 lines · Micah Gruber

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

Be careful with stuff like that, if you actually look at the code, a us == NULL doesn't seem to be possible (or usbat_flash_transport() would have oopsed before).

Signature · 1 line
Jens Axboe
#2 lore raw

tir, 04 09 2007 kl. 13:06 +0200, skrev Jens Axboe:

Quoted 9 lines · Micah Gruber
Quoted 3 lines

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

Be careful with stuff like that, if you actually look at the code, a us == NULL doesn't seem to be possible (or usbat_flash_transport() would have oopsed before).

If that is true, then

        if (!us)
                return USB_STOR_TRANSPORT_ERROR;

is utterly pointless.

Simon Holm Thøgersen

maintainer
#3 lore raw
Quoted 6 lines · Simon Holm Thøgersen
Quoted 6 lines

tir, 04 09 2007 kl. 13:06 +0200, skrev Jens Axboe:

Quoted 3 lines · Micah Gruber

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

Quoted 8 lines
Quoted 4 lines

Be careful with stuff like that, if you actually look at the code, a us == NULL doesn't seem to be possible (or usbat_flash_transport() would have oopsed before).

If that is true, then

        if (!us)
                return USB_STOR_TRANSPORT_ERROR;

is utterly pointless.

Indeed, so it is.

Alan Stern

#4 lore raw
In reply to Simon Holm Thøgersen
Quoted 6 lines · Simon Holm Thøgersen

tir, 04 09 2007 kl. 13:06 +0200, skrev Jens Axboe:

Quoted 5 lines · Micah Gruber
Quoted 3 lines

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

Quoted 8 lines
Quoted 4 lines

Be careful with stuff like that, if you actually look at the code, a us == NULL doesn't seem to be possible (or usbat_flash_transport() would have oopsed before).

If that is true, then

        if (!us)
                return USB_STOR_TRANSPORT_ERROR;

is utterly pointless.

Well that was the point I was trying to make, that test and return should be deleted instead.

Signature · 1 line
Jens Axboe
#5 lore raw

PATCH 1/1 Remove pointless NULL pointer check in drivers/usb/storage/shuttle_usbat.c.

tir, 04 09 2007 kl. 23:06 +0200, skrev Jens Axboe:

Quoted 11 lines · Simon Holm Thøgersen
Quoted 10 lines

tir, 04 09 2007 kl. 13:06 +0200, skrev Jens Axboe:

Quoted 9 lines · Micah Gruber
Quoted 3 lines

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

Be careful with stuff like that, if you actually look at the code, a us == NULL doesn't seem to be possible (or usbat_flash_transport() would have oopsed before).

Quoted 8 lines
Quoted 4 lines

If that is true, then

        if (!us)
                return USB_STOR_TRANSPORT_ERROR;

is utterly pointless.

Well that was the point I was trying to make, that test and return should be deleted instead.

I guess we agree that we want the following then.

If us would ever be NULL, the function would have oopsed already before the check.

---

drivers/usb/storage/shuttle_usbat.c +0 -3
@@ -190,9 +190,6 @@ static int usbat_check_status(struct us_data *us)
 	unsigned char *reply = us->iobuf;
 	int rc;
 
-	if (!us)
-		return USB_STOR_TRANSPORT_ERROR;
-
 	rc = usbat_get_status(us, reply);
 	if (rc != USB_STOR_XFER_GOOD)
 		return USB_STOR_TRANSPORT_FAILED;
Acked-by on v1 1/1
#6 lore raw

Re: [PATCH] Remove pointless NULL pointer check in drivers/usb/storage/shuttle_usbat.c.

Quoted 17 lines · Simon Holm Thøgersen

tir, 04 09 2007 kl. 23:06 +0200, skrev Jens Axboe:

Quoted 16 lines · Simon Holm Thøgersen
Quoted 14 lines

tir, 04 09 2007 kl. 13:06 +0200, skrev Jens Axboe:

Quoted 9 lines · Micah Gruber
Quoted 3 lines

This patch fixes a potential null dereference bug where we dereference us before a null check. This patch simply moves the dereferencing after the null check.

Be careful with stuff like that, if you actually look at the code, a us == NULL doesn't seem to be possible (or usbat_flash_transport() would have oopsed before).

If that is true, then

        if (!us)
                return USB_STOR_TRANSPORT_ERROR;

is utterly pointless.

Quoted 8 lines
Quoted 3 lines

Well that was the point I was trying to make, that test and return should be deleted instead.

I guess we agree that we want the following then.

If us would ever be NULL, the function would have oopsed already before the check.

Yep, looks much better.

Quoted 18 lines

---

drivers/usb/storage/shuttle_usbat.c +0 -3
@@ -190,9 +190,6 @@ static int usbat_check_status(struct us_data *us)
 	unsigned char *reply = us->iobuf;
 	int rc;
 
-	if (!us)
-		return USB_STOR_TRANSPORT_ERROR;
-
 	rc = usbat_get_status(us, reply);
 	if (rc != USB_STOR_XFER_GOOD)
 		return USB_STOR_TRANSPORT_FAILED;
Signature · 1 line
Jens Axboe