[PATCH][RFC] storage: Reject bogus max LUN values

[Mark Knibbs]

Hi,

Some mass storage devices return a bogus value in response to a Get Max LUN request. The Iomega Jaz USB Adapter responds with 0x10, hence my recent patch to use the US_FL_SINGLE_LUN quirk for it.

The USB MSC Bulk Only Transport document says "The device shall return one byte of data that contains the maximum LUN supported by the device."

Since the LUN field in the command block wrapper is only 4 bits wide, it might be helpful to report too-large LUN values in the kernel log, and assume max LUN is actually 0. That could get some devices which currently need the US_FL_SINGLE_LUN quirk to work.

Patch below, though whether it's worth applying might depend on whether any other devices have the same problem.


Signed-off-by: Mark Knibbs <markk-yYnZraf+/sb10XsdtD+***@public.gmane.org>

---
diff -upN linux-3.17/drivers/usb/storage/transport.c.orig linux-3.17/drivers/usb/storage/transport.c
--- linux-3.17/drivers/usb/storage/transport.c.orig 2014-10-05 20:12:36.000000000 +0100
+++ linux-3.17/drivers/usb/storage/transport.c 2014-10-12 13:11:38.000000000 +0100
@@ -1035,9 +1035,20 @@ int usb_stor_Bulk_max_lun(struct us_data
usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
result, us->iobuf[0]);

- /* if we have a successful request, return the result */
- if (result > 0)
- return us->iobuf[0];
+ /*
+ * If we have a successful request, return the result if valid. The
+ * CBW LUN field is 4 bits wide, so the value reported by the device
+ * should fit into that.
+ */
+ if (result > 0) {
+ if (!(us->iobuf[0] & 0xf0)) {
+ return us->iobuf[0];
+ } else {
+ dev_info(&us->pusb_intf->dev,
+ "Max LUN %d is not valid, using 0 instead",
+ us->iobuf[0]);
+ }
+ }

/*
* Some devices don't like GetMaxLUN. They may STALL the control
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Mark Knibbs
--- linux-3.17/drivers/usb/storage/transport.c.orig 2014-10-05 20:12:36.000000000 +0100
+++ linux-3.17/drivers/usb/storage/transport.c 2014-10-12 13:11:38.000000000 +0100
@@ -1035,9 +1035,20 @@ int usb_stor_Bulk_max_lun(struct us_data
usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
result, us->iobuf[0]);
- /* if we have a successful request, return the result */
- if (result > 0)
- return us->iobuf[0];
+ /*
+ * If we have a successful request, return the result if valid. The
+ * CBW LUN field is 4 bits wide, so the value reported by the device
+ * should fit into that.
+ */
+ if (result > 0) {
+ if (!(us->iobuf[0] & 0xf0)) {

Since the Max-LUN value is just a plain old number (with no special
interpretations for the individual bits), it is easier to understand if
the code treats it that way. Simply say:

if (us->iobuf[0] < 16) {

Post by Mark Knibbs
+ return us->iobuf[0];
+ } else {
+ dev_info(&us->pusb_intf->dev,
+ "Max LUN %d is not valid, using 0 instead",
+ us->iobuf[0]);
+ }
+ }
/*
* Some devices don't like GetMaxLUN. They may STALL the control

Apart from that minor issue,

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/***@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Matthew Dharm]

Is there a constant we can pull from a SCSI header, instead of having
a "magic number" in the code?

Matt

Post by Alan Stern

--- linux-3.17/drivers/usb/storage/transport.c.orig 2014-10-05 20:12:36.000000000 +0100
+++ linux-3.17/drivers/usb/storage/transport.c 2014-10-12 13:11:38.000000000 +0100
@@ -1035,9 +1035,20 @@ int usb_stor_Bulk_max_lun(struct us_data
usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
result, us->iobuf[0]);
- /* if we have a successful request, return the result */
- if (result > 0)
- return us->iobuf[0];
+ /*
+ * If we have a successful request, return the result if valid. The
+ * CBW LUN field is 4 bits wide, so the value reported by the device
+ * should fit into that.
+ */
+ if (result > 0) {
+ if (!(us->iobuf[0] & 0xf0)) {

Since the Max-LUN value is just a plain old number (with no special
interpretations for the individual bits), it is easier to understand if
if (us->iobuf[0] < 16) {

+ return us->iobuf[0];
+ } else {
+ dev_info(&us->pusb_intf->dev,
+ "Max LUN %d is not valid, using 0 instead",
+ us->iobuf[0]);
+ }
+ }
/*
* Some devices don't like GetMaxLUN. They may STALL the control

Apart from that minor issue,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Mark Knibbs]

On Mon, 13 Oct 2014 07:14:39 -0700

Post by Matthew Dharm
Is there a constant we can pull from a SCSI header, instead of having
a "magic number" in the code?

I don't know, but it would probably be in a USB-related header; isn't the
4-bits-for-LUN a USB mass storage bulk only thing?

I guess you could add a couple of new defines, something like
#define BOT_LUNbits 4
#define BOT_LUNmask ((1 << BOT_LUNbits) - 1)

In the patch I posted the condition could be
<= BOT_LUNmask
or
& ~BOT_LUNmask

They could also be used in usb_Stor_Bulk_transport() like this:
bcb->Lun = srb->device->lun;
if (us->fflags & US_FL_SCM_MULT_TARG)
bcb->Lun |= srb->device->id << BOT_LUNbits;
bcb->Length = srb->cmd_len;
...
usb_stor_dbg(us, "Bulk Command S 0x%x T 0x%x L %d F %d Trg %d LUN %d CL %d\n",
le32_to_cpu(bcb->Signature), bcb->Tag,
le32_to_cpu(bcb->DataTransferLength), bcb->Flags,
(bcb->Lun >> BOT_LUNbits), (bcb->Lun & BOT_LUNmask),
bcb->Length);

Though IMO it would be clearer in the call to usb_stor_dbg to use
srb->device->id instead of (bcb->Lun >> BOT_LUNbits)
and
srb->device->lun instead of (bcb->Lun & BOT_LUNmask)


Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Mark Knibbs]

Hi,

v2: Minor change as suggested

Some mass storage devices return a bogus value in response to a Get Max LUN
request. The Iomega Jaz USB Adapter responds with 0x10, hence my recent
patch to use the US_FL_SINGLE_LUN quirk for it.

The USB MSC Bulk Only Transport document says "The device shall return one
byte of data that contains the maximum LUN supported by the device."

Since the LUN field in the command block wrapper is only 4 bits wide, it
might be helpful to report too-large LUN values in the kernel log, and
assume max LUN is actually 0. That could get some devices which currently
need the US_FL_SINGLE_LUN quirk to work.


Signed-off-by: Mark Knibbs <markk-yYnZraf+/sb10XsdtD+***@public.gmane.org>
Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/***@public.gmane.org>

---
diff -upN linux-3.17/drivers/usb/storage/transport.c.orig linux-3.17/drivers/usb/storage/transport.c
--- linux-3.17/drivers/usb/storage/transport.c.orig 2014-10-05 20:12:36.000000000 +0100
+++ linux-3.17/drivers/usb/storage/transport.c 2014-10-12 13:11:38.000000000 +0100
@@ -1035,9 +1035,20 @@ int usb_stor_Bulk_max_lun(struct us_data
usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
result, us->iobuf[0]);

- /* if we have a successful request, return the result */
- if (result > 0)
- return us->iobuf[0];
+ /*
+ * If we have a successful request, return the result if valid. The
+ * CBW LUN field is 4 bits wide, so the value reported by the device
+ * should fit into that.
+ */
+ if (result > 0) {
+ if (us->iobuf[0] < 16) {
+ return us->iobuf[0];
+ } else {
+ dev_info(&us->pusb_intf->dev,
+ "Max LUN %d is not valid, using 0 instead",
+ us->iobuf[0]);
+ }
+ }

/*
* Some devices don't like GetMaxLUN. They may STALL the control
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html