1. Patchew
  2. linux
  3. View series

[PATCH] usb: storage: ene_ub6250: set transport_reset to avoid NULL deref

Nguyen Ngoc Thang posted 1 patch 2 weeks, 2 days ago
There is a newer version of this series
drivers/usb/storage/ene_ub6250.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] usb: storage: ene_ub6250: set transport_reset to avoid NULL deref
Posted by Nguyen Ngoc Thang 2 weeks, 2 days ago 
ene_ub6250_probe() sets us->transport but never sets
us->transport_reset. get_transport() only fills in transport_reset
for the standard USB_PR_CB/CBI/BULK protocols; since this driver
matches on VID/PID alone, a device can report any bInterfaceProtocol
value and still bind, leaving transport_reset NULL.

When the transport then reports an error, usb_stor_invoke_transport()
calls us->transport_reset(us) in its Handle_Errors path, dereferencing
a NULL function pointer:

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  RIP: 0010:0x0
  Call Trace:
   usb_stor_invoke_transport+0x55a/0x1a40 drivers/usb/storage/transport.c:926
   usb_stor_control_thread+0x44c/0x8f0 drivers/usb/storage/usb.c:462
   kthread+0x38b/0x480 kernel/kthread.c:436

Set transport_reset to usb_stor_Bulk_reset, matching the same pattern
already used by the other subdrivers with a custom transport function
(alauda, datafab, jumpshot, karma).

Reported-by: syzbot+356ae236154297f0d60d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=356ae236154297f0d60d
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 drivers/usb/storage/ene_ub6250.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index ed49a3bc859c..3137c2cdd021 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -2350,6 +2350,8 @@ static int ene_ub6250_probe(struct usb_interface *intf,

 	us->transport_name = "ene_ub6250";
 	us->transport = ene_transport;
+	/* get_transport() only sets this for the standard USB_PR_* protocols */
+	us->transport_reset = usb_stor_Bulk_reset;
 	us->max_lun = 0;

 	result = usb_stor_probe2(us);
--
2.43.0
Re: [PATCH] usb: storage: ene_ub6250: set transport_reset to avoid NULL deref
Posted by Alan Stern 2 weeks, 2 days ago 
On Wed, Sep 09, 2026 at 11:27:10PM +0700, Nguyen Ngoc Thang wrote:
> ene_ub6250_probe() sets us->transport but never sets
> us->transport_reset. get_transport() only fills in transport_reset
> for the standard USB_PR_CB/CBI/BULK protocols; since this driver
> matches on VID/PID alone, a device can report any bInterfaceProtocol
> value and still bind, leaving transport_reset NULL.
> 
> When the transport then reports an error, usb_stor_invoke_transport()
> calls us->transport_reset(us) in its Handle_Errors path, dereferencing
> a NULL function pointer:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   RIP: 0010:0x0
>   Call Trace:
>    usb_stor_invoke_transport+0x55a/0x1a40 drivers/usb/storage/transport.c:926
>    usb_stor_control_thread+0x44c/0x8f0 drivers/usb/storage/usb.c:462
>    kthread+0x38b/0x480 kernel/kthread.c:436
> 
> Set transport_reset to usb_stor_Bulk_reset, matching the same pattern
> already used by the other subdrivers with a custom transport function
> (alauda, datafab, jumpshot, karma).
> 
> Reported-by: syzbot+356ae236154297f0d60d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=356ae236154297f0d60d
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> ---

Is there any reason to think that usb_stor_Bulk_reset is appropriate for 
the ene_ub6250?

Probably a better solution would be either to make usb_stor_probe2() 
check that us->transport_reset isn't NULL, or even better, make 
scsiglue.c:device_reset() fail immediately if us->transport_reset is 
NULL.  Either one of these would work if some other subdriver forgets to 
set transport_reset.

Alan Stern

>  drivers/usb/storage/ene_ub6250.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
> index ed49a3bc859c..3137c2cdd021 100644
> --- a/drivers/usb/storage/ene_ub6250.c
> +++ b/drivers/usb/storage/ene_ub6250.c
> @@ -2350,6 +2350,8 @@ static int ene_ub6250_probe(struct usb_interface *intf,
> 
>  	us->transport_name = "ene_ub6250";
>  	us->transport = ene_transport;
> +	/* get_transport() only sets this for the standard USB_PR_* protocols */
> +	us->transport_reset = usb_stor_Bulk_reset;
>  	us->max_lun = 0;
> 
>  	result = usb_stor_probe2(us);
> --
> 2.43.0
>
Re: [PATCH] usb: storage: ene_ub6250: set transport_reset to avoid NULL deref
Posted by Nguyen Ngoc Thang 2 weeks, 2 days ago 
On Wed, Sep 09, 2026 at 11:59:00PM -0400, Alan Stern wrote:
> Is there any reason to think that usb_stor_Bulk_reset is appropriate for
> the ene_ub6250?

Yes: ene_ub6250 already speaks the Bulk-Only Transport wire format
underneath its own SCSI translation layer. ene_send_scsi_cmd() builds
its command/status packets with the shared struct bulk_cb_wrap /
struct bulk_cs_wrap from include/linux/usb/storage.h (same
'USBC'/'USBS' signatures, US_BULK_CB_WRAP_LEN/US_BULK_CS_WRAP_LEN
framing, and the same "0-length CSW, retry" / "CSW stalled, retry"
recovery logic as usb_stor_Bulk_transport() in transport.c). It only
uses usb_stor_bulk_transfer_buf()/usb_stor_bulk_transfer_sg() over
us->send_bulk_pipe/recv_bulk_pipe -- no control or interrupt transfer
appears anywhere in the transport. So the Bulk-Only Mass Storage Reset
request (0xFF) plus clearing halt on both bulk endpoints, which is
exactly what usb_stor_Bulk_reset() does, matches the actual wire
protocol this device uses -- it isn't a guess.

> Probably a better solution would be either to make usb_stor_probe2()
> check that us->transport_reset isn't NULL, or even better, make
> scsiglue.c:device_reset() fail immediately if us->transport_reset is
> NULL.  Either one of these would work if some other subdriver forgets
> to set transport_reset.

Agreed, that's a real gap independent of this bug -- nothing stops the
next subdriver from making the same mistake. I folded the
usb_stor_probe2() check into v2, next to the existing "transport and
protocol" check, since it's the same class of error and refusing to
probe is the safe default. Kept the ene_ub6250 transport_reset
assignment too, since that's what makes the device actually work
instead of just failing to bind.

v2 to follow.

Nguyen Ngoc Thang
[PATCH v2] usb: storage: ene_ub6250: set transport_reset to avoid NULL deref
Posted by Nguyen Ngoc Thang 2 weeks, 2 days ago 
ene_ub6250_probe() sets us->transport but never sets
us->transport_reset. get_transport() only fills in transport_reset
for the standard USB_PR_CB/CBI/BULK protocols; since this driver
matches on VID/PID alone, a device can report any bInterfaceProtocol
value and still bind, leaving transport_reset NULL.

When the transport then reports an error, usb_stor_invoke_transport()
calls us->transport_reset(us) in its Handle_Errors path, dereferencing
a NULL function pointer:

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  RIP: 0010:0x0
  Call Trace:
   usb_stor_invoke_transport+0x55a/0x1a40 drivers/usb/storage/transport.c:926
   usb_stor_control_thread+0x44c/0x8f0 drivers/usb/storage/usb.c:462
   kthread+0x38b/0x480 kernel/kthread.c:436

Set transport_reset to usb_stor_Bulk_reset: ene_send_scsi_cmd()
already frames its command/status packets with the shared
struct bulk_cb_wrap/bulk_cs_wrap from include/linux/usb/storage.h
and only ever transfers over us->send_bulk_pipe/recv_bulk_pipe, i.e.
this device speaks the Bulk-Only Transport wire protocol underneath
its own SCSI translation layer, so the Bulk-Only reset is the correct
recovery action for it, matching the pattern already used by the
other subdrivers with a custom transport function (alauda, datafab,
jumpshot, karma).

Also harden usb_stor_probe2()'s existing sanity check to require
transport_reset alongside transport and proto_handler, so a future
subdriver making the same mistake fails to probe instead of crashing
on the first error recovery.

Reported-by: syzbot+356ae236154297f0d60d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=356ae236154297f0d60d
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v2: per Alan Stern's review, justify why usb_stor_Bulk_reset is the
    correct choice for ene_ub6250, and additionally harden
    usb_stor_probe2()'s NULL check to cover transport_reset so other
    subdrivers can't hit the same bug.

 drivers/usb/storage/ene_ub6250.c | 2 ++
 drivers/usb/storage/usb.c        | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index ed49a3bc859c..3137c2cdd021 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -2350,6 +2350,8 @@ static int ene_ub6250_probe(struct usb_interface *intf,

 	us->transport_name = "ene_ub6250";
 	us->transport = ene_transport;
+	/* get_transport() only sets this for the standard USB_PR_* protocols */
+	us->transport_reset = usb_stor_Bulk_reset;
 	us->max_lun = 0;

 	result = usb_stor_probe2(us);
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 064c7fc8e368..823a4b2e9033 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1100,8 +1100,10 @@ int usb_stor_probe2(struct us_data *us)
 	int result;
 	struct device *dev = &us->pusb_intf->dev;

-	/* Make sure the transport and protocol have both been set */
-	if (!us->transport || !us->proto_handler) {
+	/* Make sure the transport, its reset method, and the protocol
+	 * have all been set
+	 */
+	if (!us->transport || !us->transport_reset || !us->proto_handler) {
 		result = -ENXIO;
 		goto BadDevice;
 	}
--
2.43.0
Re: [PATCH v2] usb: storage: ene_ub6250: set transport_reset to avoid NULL deref
Posted by Alan Stern 2 weeks, 1 day ago 
On Thu, Sep 10, 2026 at 12:19:26AM +0700, Nguyen Ngoc Thang wrote:
> ene_ub6250_probe() sets us->transport but never sets
> us->transport_reset. get_transport() only fills in transport_reset
> for the standard USB_PR_CB/CBI/BULK protocols; since this driver
> matches on VID/PID alone, a device can report any bInterfaceProtocol
> value and still bind, leaving transport_reset NULL.
> 
> When the transport then reports an error, usb_stor_invoke_transport()
> calls us->transport_reset(us) in its Handle_Errors path, dereferencing
> a NULL function pointer:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   RIP: 0010:0x0
>   Call Trace:
>    usb_stor_invoke_transport+0x55a/0x1a40 drivers/usb/storage/transport.c:926
>    usb_stor_control_thread+0x44c/0x8f0 drivers/usb/storage/usb.c:462
>    kthread+0x38b/0x480 kernel/kthread.c:436
> 
> Set transport_reset to usb_stor_Bulk_reset: ene_send_scsi_cmd()
> already frames its command/status packets with the shared
> struct bulk_cb_wrap/bulk_cs_wrap from include/linux/usb/storage.h
> and only ever transfers over us->send_bulk_pipe/recv_bulk_pipe, i.e.
> this device speaks the Bulk-Only Transport wire protocol underneath
> its own SCSI translation layer, so the Bulk-Only reset is the correct
> recovery action for it, matching the pattern already used by the
> other subdrivers with a custom transport function (alauda, datafab,
> jumpshot, karma).
> 
> Also harden usb_stor_probe2()'s existing sanity check to require
> transport_reset alongside transport and proto_handler, so a future
> subdriver making the same mistake fails to probe instead of crashing
> on the first error recovery.
> 
> Reported-by: syzbot+356ae236154297f0d60d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=356ae236154297f0d60d
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> ---
> v2: per Alan Stern's review, justify why usb_stor_Bulk_reset is the
>     correct choice for ene_ub6250, and additionally harden
>     usb_stor_probe2()'s NULL check to cover transport_reset so other
>     subdrivers can't hit the same bug.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/storage/ene_ub6250.c | 2 ++
>  drivers/usb/storage/usb.c        | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
> index ed49a3bc859c..3137c2cdd021 100644
> --- a/drivers/usb/storage/ene_ub6250.c
> +++ b/drivers/usb/storage/ene_ub6250.c
> @@ -2350,6 +2350,8 @@ static int ene_ub6250_probe(struct usb_interface *intf,
> 
>  	us->transport_name = "ene_ub6250";
>  	us->transport = ene_transport;
> +	/* get_transport() only sets this for the standard USB_PR_* protocols */
> +	us->transport_reset = usb_stor_Bulk_reset;
>  	us->max_lun = 0;
> 
>  	result = usb_stor_probe2(us);
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 064c7fc8e368..823a4b2e9033 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -1100,8 +1100,10 @@ int usb_stor_probe2(struct us_data *us)
>  	int result;
>  	struct device *dev = &us->pusb_intf->dev;
> 
> -	/* Make sure the transport and protocol have both been set */
> -	if (!us->transport || !us->proto_handler) {
> +	/* Make sure the transport, its reset method, and the protocol
> +	 * have all been set
> +	 */
> +	if (!us->transport || !us->transport_reset || !us->proto_handler) {
>  		result = -ENXIO;
>  		goto BadDevice;
>  	}
> --
> 2.43.0

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.