Bugs in scsi_vpd_inquiry()

Bugs in scsi_vpd_inquiry()

[Alan Stern]

Martin and Matthew:

Since you guys added scsi_vpd_inquiry() and scsi_get_vpd_page() plus
sd_read_block_limits() and sd_read_block_characteristics(), I'm
directing these questions to you.

Is there some reason for not accounting for the 4 header bytes in the 
allocation length value stored in the CDB?  Or is this simply a bug?

Were you aware that SCSI-2 defines the allocation length to be a single 
byte?  cmd[3] is specified as "Reserved" in the spec.  Hence the value 
of "len" should be capped at 255 if sdev->scsi_level <= SCSI_2, right?

Why does scsi_get_vpd_page() retrieve page 0 first, rather than 
directly asking for the page in question?  Is this some sort of 
play-it-safe approach, to avoid sending devices commands they may not 
support?

Have you considered that plenty of low-budget USB mass-storage devices
don't implement VPD properly?  I've got a flash drive right here which
totally ignores the "page" byte in the INQUIRY command; it always
responds with the normal INQUIRY data.  Thus expecting the response
data always to be accurate may not be a good idea.  I'm considering
adding a "restrict_to_MS_usb" flag to the host template, to indicate
that commands shouldn't be sent unless some version of Windows uses
them when talking to USB devices -- do you think that could work?

Finally, what's your opinion on the proposed patch below?

Alan Stern



Index: usb-2.6/drivers/scsi/scsi.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi.c
+++ usb-2.6/drivers/scsi/scsi.c
@@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
  * @sdev: The device to ask
  * @buffer: Where to put the result
  * @page: Which Vital Product Data to return
- * @len: The length of the buffer
+ * @len: The length of the data (= buffer length - 4)
  *
  * This is an internal helper function.  You probably want to use
  * scsi_get_vpd_page instead.
@@ -982,6 +982,12 @@ static int scsi_vpd_inquiry(struct scsi_
 	int result;
 	unsigned char cmd[16];
 
+	len += 4;		/* Include room for the header bytes */
+
+	/* SCSI-2 and earlier allow only 1 byte for the allocation length */
+	if (sdev->scsi_level <= SCSI_2)
+		len = min(len, 255u);
+
 	cmd[0] = INQUIRY;
 	cmd[1] = 1;		/* EVPD */
 	cmd[2] = page;
@@ -994,7 +1000,7 @@ static int scsi_vpd_inquiry(struct scsi_
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, NULL);
 	if (result)
 		return result;
 

Re: Bugs in scsi_vpd_inquiry()

[Matthew Wilcox]

On Mon, Aug 10, 2009 at 10:41:42AM -0400, Alan Stern wrote:
> Martin and Matthew:
> 
> Since you guys added scsi_vpd_inquiry() and scsi_get_vpd_page() plus
> sd_read_block_limits() and sd_read_block_characteristics(), I'm
> directing these questions to you.
> 
> Is there some reason for not accounting for the 4 header bytes in the 
> allocation length value stored in the CDB?  Or is this simply a bug?

Um, we do.

unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
        unsigned char *buf = kmalloc(259, GFP_KERNEL);
        result = scsi_vpd_inquiry(sdev, buf, 0, 255);
        for (i = 0; i < buf[3]; i++)
                if (buf[i + 4] == page)
                        goto found;
        buf = kmalloc(len + 4, GFP_KERNEL);
        result = scsi_vpd_inquiry(sdev, buf, page, len);

Now ... we do seem to be passing the len instead of len + 4 to the
device as the buffer size, so there does appear to be a minor bug here,
but it's not as horrific as you make it out to be.

> Were you aware that SCSI-2 defines the allocation length to be a single 
> byte?  cmd[3] is specified as "Reserved" in the spec.  Hence the value 
> of "len" should be capped at 255 if sdev->scsi_level <= SCSI_2, right?

and 'Reserved' in SCSI-2 means:

"A reserved bit, field, or byte shall be set to zero, or in accordance
with a future extension to this standard." (7.1.1)

> Why does scsi_get_vpd_page() retrieve page 0 first, rather than 
> directly asking for the page in question?  Is this some sort of 
> play-it-safe approach, to avoid sending devices commands they may not 
> support?

I think we had an example of a device which crashed when asked for pages
that it didn't support.

> Have you considered that plenty of low-budget USB mass-storage devices
> don't implement VPD properly?  I've got a flash drive right here which

I've noticed you whining about it, yes.

> totally ignores the "page" byte in the INQUIRY command; it always
> responds with the normal INQUIRY data.  Thus expecting the response

I don't think you mean the 'page' byte.  I think you mean the 'EVPD'
bit.

> data always to be accurate may not be a good idea.  I'm considering
> adding a "restrict_to_MS_usb" flag to the host template, to indicate
> that commands shouldn't be sent unless some version of Windows uses
> them when talking to USB devices -- do you think that could work?

Not really my area of expertise.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Mon, 10 Aug 2009, Matthew Wilcox wrote:

> On Mon, Aug 10, 2009 at 10:41:42AM -0400, Alan Stern wrote:
> > Martin and Matthew:
> > 
> > Since you guys added scsi_vpd_inquiry() and scsi_get_vpd_page() plus
> > sd_read_block_limits() and sd_read_block_characteristics(), I'm
> > directing these questions to you.
> > 
> > Is there some reason for not accounting for the 4 header bytes in the 
> > allocation length value stored in the CDB?  Or is this simply a bug?
> 
> Um, we do.
> 
> unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
>         unsigned char *buf = kmalloc(259, GFP_KERNEL);
>         result = scsi_vpd_inquiry(sdev, buf, 0, 255);
>         for (i = 0; i < buf[3]; i++)
>                 if (buf[i + 4] == page)
>                         goto found;
>         buf = kmalloc(len + 4, GFP_KERNEL);
>         result = scsi_vpd_inquiry(sdev, buf, page, len);

I'm not referring to this routine; I'm talking about the code in 
scsi_vpd_inquiry() where the CDB is set.

> Now ... we do seem to be passing the len instead of len + 4 to the
> device as the buffer size,

Yes, that's what I mean.

> so there does appear to be a minor bug here,
> but it's not as horrific as you make it out to be.

I didn't make it out to be horrific; I said that it's "simply a bug".  
And you just agreed with me.  (Well, you called it a "minor" bug; I'll 
go along with that.)


> > Were you aware that SCSI-2 defines the allocation length to be a single 
> > byte?  cmd[3] is specified as "Reserved" in the spec.  Hence the value 
> > of "len" should be capped at 255 if sdev->scsi_level <= SCSI_2, right?
> 
> and 'Reserved' in SCSI-2 means:
> 
> "A reserved bit, field, or byte shall be set to zero, or in accordance
> with a future extension to this standard." (7.1.1)

Sure.  But what reason could there possibly be for making a field
non-zero when you know that the device won't be able to interpret the
value correctly?  The fact that sdev->scsi_level == SCSI_2 means
the device follows _this_ version of the standard, not a future 
extension.  Or am I missing something?


> > Have you considered that plenty of low-budget USB mass-storage devices
> > don't implement VPD properly?  I've got a flash drive right here which
> 
> I've noticed you whining about it, yes.

Have I mentioned it before?  I don't recall doing so...  Maybe my 
memory is going.

(And there's no reason to be rude.  I'm trying to hold a civil
discussion.)

> > totally ignores the "page" byte in the INQUIRY command; it always
> > responds with the normal INQUIRY data.  Thus expecting the response
> 
> I don't think you mean the 'page' byte.  I think you mean the 'EVPD'
> bit.

That's right.  Sorry about the mental slip-up.

> > data always to be accurate may not be a good idea.  I'm considering
> > adding a "restrict_to_MS_usb" flag to the host template, to indicate
> > that commands shouldn't be sent unless some version of Windows uses
> > them when talking to USB devices -- do you think that could work?
> 
> Not really my area of expertise.

Okay.  Maybe Martin has some thoughts on it.

You didn't comment on the proposed patch.  Any objections to it?

Alan Stern

Re: Bugs in scsi_vpd_inquiry()

[Martin K. Petersen]

>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan,

>> > data always to be accurate may not be a good idea.  I'm considering
>> > adding a "restrict_to_MS_usb" flag to the host template, to
>> > indicate that commands shouldn't be sent unless some version of
>> > Windows uses them when talking to USB devices -- do you think that
>> > could work?
>> 
>> Not really my area of expertise.

Alan> Okay.  Maybe Martin has some thoughts on it.

First of all we're not going to send EVPD=1 out to devices reporting
SCSI_SPC_2 or lower anymore, making some of this discussion moot in the
short term.

But as I have alluded to in the past we do have a conflict brewing
because the switch to drives with 4KB physical blocks will mean USB
bridges will have to get smarter.  And that in turn will mean adhering
(*gasp*) to the standard instead of firmware writers flipping bits until
Windows stops crashing.  Windows 7 does in fact query drives about
alignment, block sizes, etc.  But I'm not sure how true that is for the
Windows USB storage stack.

Originally I was in favor of just considering any USB device suspect and
only send it a limited command set.  But the 4KB switch means that it is
imperative that we get the alignment reported correctly.  Performance is
totally and absolutely going to tank without it.  Enough that users will
complain loudly.

Consequently, I'm no longer a fan of the reduced command set approach.
Because in 12 months that MS command set *will* include stuff that
causes existing USB devices to crash, catch fire, or eat your breakfast.

And then we're back to finding heuristics for USB device brain damage.
Blindly clamping the SCSI rev. in scsiglue.c will have to be replaced
with something smarter.  And I really think that's what needs to be
fixed.

Please note that I'm not trying to weasel out of changing stuff in SCSI.
But it's USB devices that deviate wildly from the spec, so I think we
should keep as much of the workaround stuff in the USB stack as
possible.  And the current SCSI rev. mechanism is a reasonable way to
limit what we send out in sd.c.

So I'd prefer an approach where the USB stack sets the SCSI level
according to how much it trusts the attached device.

Is there a USB rev. or something else you can key off of and combine
with the device-reported SCSI rev. to come up with a better heuristic?
Something which doesn't depend on being SCSIly correct.

Do we have any way of identifying the devices that caused the SCSI
rev. to be clamped for USB in the first place?  Any way of collecting
that data over a couple of kernel release cycles?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Mon, 10 Aug 2009, Martin K. Petersen wrote:

> >>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:
> 
> Alan,
> 
> >> > data always to be accurate may not be a good idea.  I'm considering
> >> > adding a "restrict_to_MS_usb" flag to the host template, to
> >> > indicate that commands shouldn't be sent unless some version of
> >> > Windows uses them when talking to USB devices -- do you think that
> >> > could work?
> >> 
> >> Not really my area of expertise.
> 
> Alan> Okay.  Maybe Martin has some thoughts on it.
> 
> First of all we're not going to send EVPD=1 out to devices reporting
> SCSI_SPC_2 or lower anymore, making some of this discussion moot in the
> short term.

Ah.  Has this change been posted anywhere yet?

> But as I have alluded to in the past we do have a conflict brewing
> because the switch to drives with 4KB physical blocks will mean USB
> bridges will have to get smarter.

If only all the existing bridges could be made smarter too!  :-)

>  And that in turn will mean adhering
> (*gasp*) to the standard instead of firmware writers flipping bits until
> Windows stops crashing.  Windows 7 does in fact query drives about
> alignment, block sizes, etc.  But I'm not sure how true that is for the
> Windows USB storage stack.

Does Vista also do this querying?  I've got access to a machine running 
Vista, so I can test the USB storage stack behavior.  But I don't have 
access to any machines with Windows 7.

> Originally I was in favor of just considering any USB device suspect and
> only send it a limited command set.  But the 4KB switch means that it is
> imperative that we get the alignment reported correctly.  Performance is
> totally and absolutely going to tank without it.  Enough that users will
> complain loudly.
> 
> Consequently, I'm no longer a fan of the reduced command set approach.
> Because in 12 months that MS command set *will* include stuff that
> causes existing USB devices to crash, catch fire, or eat your breakfast.

How will Windows 7 deal with all those old buggy USB devices?  (You 
probably don't have any idea...)

> And then we're back to finding heuristics for USB device brain damage.
> Blindly clamping the SCSI rev. in scsiglue.c will have to be replaced
> with something smarter.  And I really think that's what needs to be
> fixed.

I generally agree.  But finding a solution won't be easy.

> Please note that I'm not trying to weasel out of changing stuff in SCSI.
> But it's USB devices that deviate wildly from the spec, so I think we
> should keep as much of the workaround stuff in the USB stack as
> possible.  And the current SCSI rev. mechanism is a reasonable way to
> limit what we send out in sd.c.
> 
> So I'd prefer an approach where the USB stack sets the SCSI level
> according to how much it trusts the attached device.
> 
> Is there a USB rev. or something else you can key off of and combine
> with the device-reported SCSI rev. to come up with a better heuristic?
> Something which doesn't depend on being SCSIly correct.

No, there isn't.  There are USB device IDs, but they are assigned more 
or less randomly by the manufacturers -- they don't form a nicely 
increasing sequence of revision numbers.

Even worse, the device-reported SCSI rev. is completely independent of
the ID for the bridge chip, which is where the failures seem to occur.  
(Although it's hard to be certain which component is failing.)

> Do we have any way of identifying the devices that caused the SCSI
> rev. to be clamped for USB in the first place?  Any way of collecting
> that data over a couple of kernel release cycles?

Some of them are listed in the email archives.  Searching through some
old records I came across this example:

	http://marc.info/?l=linux-scsi&m=110895748728466&w=2

Unfortunately this thread doesn't contain the original bug report with
the USB device ID; I think the files were put up on a web server that 
doesn't exist any more.

I'm not keen on the idea of collecting more failure data by changing
the driver so as to cause failures!

Alan Stern

Re: Bugs in scsi_vpd_inquiry()

[Martin K. Petersen]

>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

>> First of all we're not going to send EVPD=1 out to devices reporting
>> SCSI_SPC_2 or lower anymore, making some of this discussion moot in
>> the short term.

Alan> Ah.  Has this change been posted anywhere yet?

ffd4bc2a984fab40ed969163efdff321490e8032


Alan> Does Vista also do this querying?  I've got access to a machine
Alan> running Vista, so I can test the USB storage stack behavior.

Dunno.


Alan> How will Windows 7 deal with all those old buggy USB devices?
Alan> (You probably don't have any idea...)

Nope :)


Alan> No, there isn't.  There are USB device IDs, but they are assigned
Alan> more or less randomly by the manufacturers -- they don't form a
Alan> nicely increasing sequence of revision numbers.

But let's assume for a second that USB drives with 4KB physical blocks
will be USB 3.0.  I think that's a likely scenario.  Couldn't we do
something like?

   /* Dumb down SCSI level for USB 1.1 and 2.0 devices */
   if (le16_to_cpu(udev->descriptor.bcdUSB) <= 0x0200 
       && sdev->scsi_level > SCSI_2)
          sdev->sdev_target->scsi_level = sdev->scsi_level = SCSI_2;

I'm sure that won't catch all devices.  But it would at least be a step
in the right direction.  And if that breaks we can revisit.


Alan> Even worse, the device-reported SCSI rev. is completely
Alan> independent of the ID for the bridge chip, which is where the
Alan> failures seem to occur.  (Although it's hard to be certain which
Alan> component is failing.)

If the bridge doesn't provide the SCSI rev. where does it come from?  Or
are you saying there's a USB "target" chip and then a USB-SATA bridge
chip behind it?


Alan> I'm not keen on the idea of collecting more failure data by
Alan> changing the driver so as to cause failures!

I wasn't thinking a hard fail.  More like a:

  printk(KERN_ERR "Please mail this info to linux-scsi@vger...");

and have that sit in Fedora/Ubuntu for 6 months.

But if there's nothing we can key off of then there's no point.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Mon, 10 Aug 2009, Martin K. Petersen wrote:

> >>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:
> 
> >> First of all we're not going to send EVPD=1 out to devices reporting
> >> SCSI_SPC_2 or lower anymore, making some of this discussion moot in
> >> the short term.
> 
> Alan> Ah.  Has this change been posted anywhere yet?
> 
> ffd4bc2a984fab40ed969163efdff321490e8032

Looks good, thanks.

> But let's assume for a second that USB drives with 4KB physical blocks
> will be USB 3.0.  I think that's a likely scenario.  Couldn't we do
> something like?
> 
>    /* Dumb down SCSI level for USB 1.1 and 2.0 devices */
>    if (le16_to_cpu(udev->descriptor.bcdUSB) <= 0x0200 
>        && sdev->scsi_level > SCSI_2)
>           sdev->sdev_target->scsi_level = sdev->scsi_level = SCSI_2;
> 
> I'm sure that won't catch all devices.  But it would at least be a step
> in the right direction.  And if that breaks we can revisit.

That's a good idea.  At least it won't break any existing devices, 
although some new ones may fail.


> Alan> Even worse, the device-reported SCSI rev. is completely
> Alan> independent of the ID for the bridge chip, which is where the
> Alan> failures seem to occur.  (Although it's hard to be certain which
> Alan> component is failing.)
> 
> If the bridge doesn't provide the SCSI rev. where does it come from?  Or
> are you saying there's a USB "target" chip and then a USB-SATA bridge
> chip behind it?

There's the bridge chip and the drive itself.  The drive provides the 
INQUIRY data and the chip provides the USB identifiers.

The same bridge chip can be used with lots of different drives.  In
fact, you can buy a USB-(S)ATA converter and plug it into whatever
drives you have lying around.

> Alan> I'm not keen on the idea of collecting more failure data by
> Alan> changing the driver so as to cause failures!
> 
> I wasn't thinking a hard fail.  More like a:
> 
>   printk(KERN_ERR "Please mail this info to linux-scsi@vger...");
> 
> and have that sit in Fedora/Ubuntu for 6 months.
> 
> But if there's nothing we can key off of then there's no point.

About the only thing to key off of is failure of the device when we 
send it a REPORT LUNS command.  :-(

Alan Stern

Re: Bugs in scsi_vpd_inquiry()

[Douglas Gilbert]

Martin K. Petersen wrote:
>>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:
> 
> Alan,
> 
>>>> data always to be accurate may not be a good idea.  I'm considering
>>>> adding a "restrict_to_MS_usb" flag to the host template, to
>>>> indicate that commands shouldn't be sent unless some version of
>>>> Windows uses them when talking to USB devices -- do you think that
>>>> could work?
>>> Not really my area of expertise.
> 
> Alan> Okay.  Maybe Martin has some thoughts on it.
> 
> First of all we're not going to send EVPD=1 out to devices reporting
> SCSI_SPC_2 or lower anymore, making some of this discussion moot in the
> short term.
> 
> But as I have alluded to in the past we do have a conflict brewing
> because the switch to drives with 4KB physical blocks will mean USB
> bridges will have to get smarter.  And that in turn will mean adhering
> (*gasp*) to the standard instead of firmware writers flipping bits until
> Windows stops crashing.  Windows 7 does in fact query drives about
> alignment, block sizes, etc.  But I'm not sure how true that is for the
> Windows USB storage stack.

It would be interesting to know if Windows 7 is pushing
UAS (USB attached SCSI) [latest draft at www.t10.org is
uas-r02.pdf].

"USB Attached SCSI is a new generation of USB Transport
  Standards. This standard supports the following
  features in support of USB-20 and future USB
  specifications:
     a)  does not interfere with the USB Mass Storage Class
         (MSC) bulk-only transport;
     b)  mechanism to send commands associated with any T10
         standard to a USB device;
     c)  support for queuing in the protocol;
     d)  support for autosense;
     e)  compliance with SCSI Architecture Model - 4 (SAM-4)
         or later; and
     f)  other capabilities."

It is at the letter ballot stage at t10.


Doug Gilbert

Re: Bugs in scsi_vpd_inquiry()

[Martin K. Petersen]

>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan,

>> If the bridge doesn't provide the SCSI rev. where does it come from?
>> Or are you saying there's a USB "target" chip and then a USB-SATA
>> bridge chip behind it?

Alan> There's the bridge chip and the drive itself.  The drive provides
Alan> the INQUIRY data and the chip provides the USB identifiers.

But the drive is ATA.  It must be the bridge that translates whatever
the drive reports in IDENTIFY DEVICE into something that makes sense in
the SPC/SBC universe.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: Bugs in scsi_vpd_inquiry()

[Martin K. Petersen]

>>>>> "Doug" == Douglas Gilbert <dgilbert@interlog.com> writes:

Doug> It would be interesting to know if Windows 7 is pushing UAS (USB
Doug> attached SCSI) [latest draft at www.t10.org is uas-r02.pdf].

I'll see what I can dig up...

In any case UAS is a transport class.  Sure, it would provide us with
another "can't-be-completely-brain-dead" heuristic.  But we're still at
the firmware writer's whim when it comes to SPC/SBC-level commands.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: Bugs in scsi_vpd_inquiry()

[Boaz Harrosh]

On 08/10/2009 05:41 PM, Alan Stern wrote:
> Martin and Matthew:
> 
> Since you guys added scsi_vpd_inquiry() and scsi_get_vpd_page() plus
> sd_read_block_limits() and sd_read_block_characteristics(), I'm
> directing these questions to you.
> 
> Is there some reason for not accounting for the 4 header bytes in the 
> allocation length value stored in the CDB?  Or is this simply a bug?
> 
> Were you aware that SCSI-2 defines the allocation length to be a single 
> byte?  cmd[3] is specified as "Reserved" in the spec.  Hence the value 
> of "len" should be capped at 255 if sdev->scsi_level <= SCSI_2, right?
> 
> Why does scsi_get_vpd_page() retrieve page 0 first, rather than 
> directly asking for the page in question?  Is this some sort of 
> play-it-safe approach, to avoid sending devices commands they may not 
> support?
> 
> Have you considered that plenty of low-budget USB mass-storage devices
> don't implement VPD properly?  I've got a flash drive right here which
> totally ignores the "page" byte in the INQUIRY command; it always
> responds with the normal INQUIRY data.  Thus expecting the response
> data always to be accurate may not be a good idea.  I'm considering
> adding a "restrict_to_MS_usb" flag to the host template, to indicate
> that commands shouldn't be sent unless some version of Windows uses
> them when talking to USB devices -- do you think that could work?
> 
> Finally, what's your opinion on the proposed patch below?
> 
> Alan Stern
> 
> 
> 
> Index: usb-2.6/drivers/scsi/scsi.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi.c
> +++ usb-2.6/drivers/scsi/scsi.c
> @@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
>   * @sdev: The device to ask
>   * @buffer: Where to put the result
>   * @page: Which Vital Product Data to return
> - * @len: The length of the buffer
> + * @len: The length of the data (= buffer length - 4)
>   *
>   * This is an internal helper function.  You probably want to use
>   * scsi_get_vpd_page instead.
> @@ -982,6 +982,12 @@ static int scsi_vpd_inquiry(struct scsi_
>  	int result;
>  	unsigned char cmd[16];
>  
> +	len += 4;		/* Include room for the header bytes */
> +
> +	/* SCSI-2 and earlier allow only 1 byte for the allocation length */
> +	if (sdev->scsi_level <= SCSI_2)
> +		len = min(len, 255u);
> +
>  	cmd[0] = INQUIRY;
>  	cmd[1] = 1;		/* EVPD */
>  	cmd[2] = page;
> @@ -994,7 +1000,7 @@ static int scsi_vpd_inquiry(struct scsi_
>  	 * all the existing users tried this hard.
>  	 */
>  	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
> -				  len + 4, NULL, 30 * HZ, 3, NULL);
> +				  len, NULL, 30 * HZ, 3, NULL);
>  	if (result)
>  		return result;
>  
> 

This is certainly a bug. Otherwise I would get all my pages 4 bytes short
and wonder why.

I wish the bug would explain that stupid USB device Martin was fixing.
"I die if evpd page=0 is read" is a very brain dead thing. But there
is no overflow in current code, only underflow.

If you are at it could you please fix all the bugs in this code: ;-)

---
git diff --stat -p drivers/scsi/scsi.c
 drivers/scsi/scsi.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2de5f3a..aca26a1 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -982,6 +982,14 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	int result;
 	unsigned char cmd[16];
 
+	buffer[1] = ~page;
+
+	len += 4;		/* Include room for the header bytes */
+
+	/* SCSI-2 and earlier allow only 1 byte for the allocation length */
+	if (sdev->scsi_level <= SCSI_2)
+		len = min(len, 255u);
+
 	cmd[0] = INQUIRY;
 	cmd[1] = 1;		/* EVPD */
 	cmd[2] = page;
@@ -994,7 +1002,7 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, NULL);
 	if (result)
 		return result;
 

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Mon, 10 Aug 2009, Martin K. Petersen wrote:

> >>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:
> 
> Alan,
> 
> >> If the bridge doesn't provide the SCSI rev. where does it come from?
> >> Or are you saying there's a USB "target" chip and then a USB-SATA
> >> bridge chip behind it?
> 
> Alan> There's the bridge chip and the drive itself.  The drive provides
> Alan> the INQUIRY data and the chip provides the USB identifiers.
> 
> But the drive is ATA.  It must be the bridge that translates whatever
> the drive reports in IDENTIFY DEVICE into something that makes sense in
> the SPC/SBC universe.

Right.  The drive provides the data, and the bridge massages it into
the form of an INQUIRY response and sends it back to the host.

Alan Stern

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Tue, 11 Aug 2009, Boaz Harrosh wrote:

> This is certainly a bug. Otherwise I would get all my pages 4 bytes short
> and wonder why.
> 
> I wish the bug would explain that stupid USB device Martin was fixing.
> "I die if evpd page=0 is read" is a very brain dead thing. But there
> is no overflow in current code, only underflow.
> 
> If you are at it could you please fix all the bugs in this code: ;-)

The USB problem shouldn't affect anything thanks to Martin's other
changes (sd won't read VPD for devices with scsi_level <= SCSI_2).  So
how does this revised patch look?

Alan Stern


Index: usb-2.6/drivers/scsi/scsi.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi.c
+++ usb-2.6/drivers/scsi/scsi.c
@@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
  * @sdev: The device to ask
  * @buffer: Where to put the result
  * @page: Which Vital Product Data to return
- * @len: The length of the buffer
+ * @len: The length of the data (= buffer length - 4)
  *
  * This is an internal helper function.  You probably want to use
  * scsi_get_vpd_page instead.
@@ -980,7 +980,10 @@ static int scsi_vpd_inquiry(struct scsi_
 							u8 page, unsigned len)
 {
 	int result;
-	unsigned char cmd[16];
+	int resid;
+	unsigned char cmd[6];
+
+	len += 4;		/* Include room for the header bytes */
 
 	cmd[0] = INQUIRY;
 	cmd[1] = 1;		/* EVPD */
@@ -989,17 +992,19 @@ static int scsi_vpd_inquiry(struct scsi_
 	cmd[4] = len & 0xff;
 	cmd[5] = 0;		/* Control byte */
 
+	buffer[1] = ~page;
+
 	/*
 	 * I'm not convinced we need to try quite this hard to get VPD, but
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, &resid);
 	if (result)
 		return result;
 
-	/* Sanity check that we got the page back that we asked for */
-	if (buffer[1] != page)
+	/* Sanity check that we got the header and the page we asked for */
+	if (resid > len - 4 || buffer[1] != page)
 		return -EIO;
 
 	return 0;

Re: Bugs in scsi_vpd_inquiry()

[James Bottomley]

On Tue, 2009-08-11 at 10:53 -0400, Alan Stern wrote:
> On Tue, 11 Aug 2009, Boaz Harrosh wrote:
> 
> > This is certainly a bug. Otherwise I would get all my pages 4 bytes short
> > and wonder why.
> > 
> > I wish the bug would explain that stupid USB device Martin was fixing.
> > "I die if evpd page=0 is read" is a very brain dead thing. But there
> > is no overflow in current code, only underflow.
> > 
> > If you are at it could you please fix all the bugs in this code: ;-)
> 
> The USB problem shouldn't affect anything thanks to Martin's other
> changes (sd won't read VPD for devices with scsi_level <= SCSI_2).  So
> how does this revised patch look?
> 
> Alan Stern
> 
> 
> Index: usb-2.6/drivers/scsi/scsi.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi.c
> +++ usb-2.6/drivers/scsi/scsi.c
> @@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
>   * @sdev: The device to ask
>   * @buffer: Where to put the result
>   * @page: Which Vital Product Data to return
> - * @len: The length of the buffer
> + * @len: The length of the data (= buffer length - 4)

Really, no.  The former is the correct (and universally used
definition).  This one you propose is asking for confusion and misuse.

>   *
>   * This is an internal helper function.  You probably want to use
>   * scsi_get_vpd_page instead.
> @@ -980,7 +980,10 @@ static int scsi_vpd_inquiry(struct scsi_
>  							u8 page, unsigned len)
>  {
>  	int result;
> -	unsigned char cmd[16];
> +	int resid;
> +	unsigned char cmd[6];
> +
> +	len += 4;		/* Include room for the header bytes */
>  
>  	cmd[0] = INQUIRY;
>  	cmd[1] = 1;		/* EVPD */
> @@ -989,17 +992,19 @@ static int scsi_vpd_inquiry(struct scsi_
>  	cmd[4] = len & 0xff;
>  	cmd[5] = 0;		/* Control byte */
>  
> +	buffer[1] = ~page;

This is pointless and dangerous:  Some architectures will invalidate
caches for DMA not flush them, so it might not do what you think it
does.

James

Re: Bugs in scsi_vpd_inquiry()

[Boaz Harrosh]

On 08/11/2009 06:13 PM, James Bottomley wrote:
> On Tue, 2009-08-11 at 10:53 -0400, Alan Stern wrote:
>> On Tue, 11 Aug 2009, Boaz Harrosh wrote:
>>
>>> This is certainly a bug. Otherwise I would get all my pages 4 bytes short
>>> and wonder why.
>>>
>>> I wish the bug would explain that stupid USB device Martin was fixing.
>>> "I die if evpd page=0 is read" is a very brain dead thing. But there
>>> is no overflow in current code, only underflow.
>>>
>>> If you are at it could you please fix all the bugs in this code: ;-)
>>
>> The USB problem shouldn't affect anything thanks to Martin's other
>> changes (sd won't read VPD for devices with scsi_level <= SCSI_2).  So
>> how does this revised patch look?
>>
>> Alan Stern
>>
>>
>> Index: usb-2.6/drivers/scsi/scsi.c
>> ===================================================================
>> --- usb-2.6.orig/drivers/scsi/scsi.c
>> +++ usb-2.6/drivers/scsi/scsi.c
>> @@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
>>   * @sdev: The device to ask
>>   * @buffer: Where to put the result
>>   * @page: Which Vital Product Data to return
>> - * @len: The length of the buffer
>> + * @len: The length of the data (= buffer length - 4)
> 
> Really, no.  The former is the correct (and universally used
> definition).  This one you propose is asking for confusion and misuse.
> 

This is what is done today Only documented

If you want to fix it to be the full buffer size the user code
should be fixed

>>   *
>>   * This is an internal helper function.  You probably want to use
>>   * scsi_get_vpd_page instead.
>> @@ -980,7 +980,10 @@ static int scsi_vpd_inquiry(struct scsi_
>>  							u8 page, unsigned len)
>>  {
>>  	int result;
>> -	unsigned char cmd[16];
>> +	int resid;
>> +	unsigned char cmd[6];
>> +
>> +	len += 4;		/* Include room for the header bytes */
>>  
>>  	cmd[0] = INQUIRY;
>>  	cmd[1] = 1;		/* EVPD */
>> @@ -989,17 +992,19 @@ static int scsi_vpd_inquiry(struct scsi_
>>  	cmd[4] = len & 0xff;
>>  	cmd[5] = 0;		/* Control byte */
>>  
>> +	buffer[1] = ~page;
> 
> This is pointless and dangerous:  Some architectures will invalidate
> caches for DMA not flush them, so it might not do what you think it
> does.
> 

Then you can't do that "after check" either. It's a simple minefield.
What do you suggest?

> James
> 
> 

Boaz

Re: Bugs in scsi_vpd_inquiry()

[James Bottomley]

On Tue, 2009-08-11 at 18:18 +0300, Boaz Harrosh wrote:
> On 08/11/2009 06:13 PM, James Bottomley wrote:
> > On Tue, 2009-08-11 at 10:53 -0400, Alan Stern wrote:
> >> On Tue, 11 Aug 2009, Boaz Harrosh wrote:
> >>
> >>> This is certainly a bug. Otherwise I would get all my pages 4 bytes short
> >>> and wonder why.
> >>>
> >>> I wish the bug would explain that stupid USB device Martin was fixing.
> >>> "I die if evpd page=0 is read" is a very brain dead thing. But there
> >>> is no overflow in current code, only underflow.
> >>>
> >>> If you are at it could you please fix all the bugs in this code: ;-)
> >>
> >> The USB problem shouldn't affect anything thanks to Martin's other
> >> changes (sd won't read VPD for devices with scsi_level <= SCSI_2).  So
> >> how does this revised patch look?
> >>
> >> Alan Stern
> >>
> >>
> >> Index: usb-2.6/drivers/scsi/scsi.c
> >> ===================================================================
> >> --- usb-2.6.orig/drivers/scsi/scsi.c
> >> +++ usb-2.6/drivers/scsi/scsi.c
> >> @@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
> >>   * @sdev: The device to ask
> >>   * @buffer: Where to put the result
> >>   * @page: Which Vital Product Data to return
> >> - * @len: The length of the buffer
> >> + * @len: The length of the data (= buffer length - 4)
> > 
> > Really, no.  The former is the correct (and universally used
> > definition).  This one you propose is asking for confusion and misuse.
> > 
> 
> This is what is done today Only documented
> 
> If you want to fix it to be the full buffer size the user code
> should be fixed

Right.

> >>   *
> >>   * This is an internal helper function.  You probably want to use
> >>   * scsi_get_vpd_page instead.
> >> @@ -980,7 +980,10 @@ static int scsi_vpd_inquiry(struct scsi_
> >>  							u8 page, unsigned len)
> >>  {
> >>  	int result;
> >> -	unsigned char cmd[16];
> >> +	int resid;
> >> +	unsigned char cmd[6];
> >> +
> >> +	len += 4;		/* Include room for the header bytes */
> >>  
> >>  	cmd[0] = INQUIRY;
> >>  	cmd[1] = 1;		/* EVPD */
> >> @@ -989,17 +992,19 @@ static int scsi_vpd_inquiry(struct scsi_
> >>  	cmd[4] = len & 0xff;
> >>  	cmd[5] = 0;		/* Control byte */
> >>  
> >> +	buffer[1] = ~page;
> > 
> > This is pointless and dangerous:  Some architectures will invalidate
> > caches for DMA not flush them, so it might not do what you think it
> > does.
> > 
> 
> Then you can't do that "after check" either. It's a simple minefield.
> What do you suggest?

Well, nothing really, like the original code.  Byzantine checking is
never a good idea.  You always assume that if a device told you it did
what you told it, it actually did.  If we find devices that fail this
simple premise, then we get into blacklists and other forms of
nastiness.

James

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Tue, 11 Aug 2009, James Bottomley wrote:

> > > This is pointless and dangerous:  Some architectures will invalidate
> > > caches for DMA not flush them, so it might not do what you think it
> > > does.
> > > 
> > 
> > Then you can't do that "after check" either. It's a simple minefield.
> > What do you suggest?
> 
> Well, nothing really, like the original code.  Byzantine checking is
> never a good idea.  You always assume that if a device told you it did
> what you told it, it actually did.  If we find devices that fail this
> simple premise, then we get into blacklists and other forms of
> nastiness.

Okay, then how about this?

Alan Stern



Index: usb-2.6/drivers/scsi/scsi.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi.c
+++ usb-2.6/drivers/scsi/scsi.c
@@ -980,7 +980,8 @@ static int scsi_vpd_inquiry(struct scsi_
 							u8 page, unsigned len)
 {
 	int result;
-	unsigned char cmd[16];
+	int resid;
+	unsigned char cmd[6];
 
 	cmd[0] = INQUIRY;
 	cmd[1] = 1;		/* EVPD */
@@ -994,12 +995,12 @@ static int scsi_vpd_inquiry(struct scsi_
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, &resid);
 	if (result)
 		return result;
 
-	/* Sanity check that we got the page back that we asked for */
-	if (buffer[1] != page)
+	/* Sanity check that we at least got the header */
+	if (resid > len - 4)
 		return -EIO;
 
 	return 0;
@@ -1027,7 +1028,7 @@ unsigned char *scsi_get_vpd_page(struct 
 		return NULL;
 
 	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
+	result = scsi_vpd_inquiry(sdev, buf, 0, 255 + 4);
 	if (result)
 		goto fail;
 
@@ -1042,7 +1043,7 @@ unsigned char *scsi_get_vpd_page(struct 
 	goto fail;
 
  found:
-	result = scsi_vpd_inquiry(sdev, buf, page, 255);
+	result = scsi_vpd_inquiry(sdev, buf, page, 255 + 4);
 	if (result)
 		goto fail;
 
@@ -1056,7 +1057,7 @@ unsigned char *scsi_get_vpd_page(struct 
 
 	kfree(buf);
 	buf = kmalloc(len + 4, GFP_KERNEL);
-	result = scsi_vpd_inquiry(sdev, buf, page, len);
+	result = scsi_vpd_inquiry(sdev, buf, page, len + 4);
 	if (result)
 		goto fail;
 

Re: Bugs in scsi_vpd_inquiry()

[Matthew Wilcox]

On Tue, Aug 11, 2009 at 11:38:03AM -0400, Alan Stern wrote:
> Okay, then how about this?

Crap.  Try this instead.

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2de5f3a..e39d00a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -994,7 +994,7 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, NULL);
 	if (result)
 		return result;
 
@@ -1020,14 +1020,20 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
 {
 	int i, result;
-	unsigned int len;
-	unsigned char *buf = kmalloc(259, GFP_KERNEL);
+	unsigned int len, alloc;
+	unsigned char *buf;
+
+	/* SCSI-2 only permits 255 bytes of information to be provided */
+	alloc = 259;
+	if (sdev->scsi_level <= SCSI_2)
+		alloc = 255;
 
+	buf = kmalloc(alloc, GFP_KERNEL);
 	if (!buf)
 		return NULL;
 
 	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
+	result = scsi_vpd_inquiry(sdev, buf, 0, alloc);
 	if (result)
 		goto fail;
 
@@ -1042,7 +1048,7 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
 	goto fail;
 
  found:
-	result = scsi_vpd_inquiry(sdev, buf, page, 255);
+	result = scsi_vpd_inquiry(sdev, buf, page, alloc);
 	if (result)
 		goto fail;
 
@@ -1050,12 +1056,12 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
 	 * Some pages are longer than 255 bytes.  The actual length of
 	 * the page is returned in the header.
 	 */
-	len = (buf[2] << 8) | buf[3];
-	if (len <= 255)
+	len = ((buf[2] << 8) | buf[3]) + 4;
+	if (len <= alloc)
 		return buf;
 
 	kfree(buf);
-	buf = kmalloc(len + 4, GFP_KERNEL);
+	buf = kmalloc(len, GFP_KERNEL);
 	result = scsi_vpd_inquiry(sdev, buf, page, len);
 	if (result)
 		goto fail;

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Bugs in scsi_vpd_inquiry()

[James Bottomley]

On Tue, 2009-08-11 at 11:38 -0400, Alan Stern wrote:
> On Tue, 11 Aug 2009, James Bottomley wrote:
> 
> > > > This is pointless and dangerous:  Some architectures will invalidate
> > > > caches for DMA not flush them, so it might not do what you think it
> > > > does.
> > > > 
> > > 
> > > Then you can't do that "after check" either. It's a simple minefield.
> > > What do you suggest?
> > 
> > Well, nothing really, like the original code.  Byzantine checking is
> > never a good idea.  You always assume that if a device told you it did
> > what you told it, it actually did.  If we find devices that fail this
> > simple premise, then we get into blacklists and other forms of
> > nastiness.
> 
> Okay, then how about this?
> 
> Alan Stern
> 
> 
> 
> Index: usb-2.6/drivers/scsi/scsi.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi.c
> +++ usb-2.6/drivers/scsi/scsi.c
> @@ -980,7 +980,8 @@ static int scsi_vpd_inquiry(struct scsi_
>  							u8 page, unsigned len)
>  {
>  	int result;
> -	unsigned char cmd[16];
> +	int resid;
> +	unsigned char cmd[6];
>  
>  	cmd[0] = INQUIRY;
>  	cmd[1] = 1;		/* EVPD */
> @@ -994,12 +995,12 @@ static int scsi_vpd_inquiry(struct scsi_
>  	 * all the existing users tried this hard.
>  	 */
>  	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
> -				  len + 4, NULL, 30 * HZ, 3, NULL);
> +				  len, NULL, 30 * HZ, 3, &resid);
>  	if (result)
>  		return result;
>  
> -	/* Sanity check that we got the page back that we asked for */
> -	if (buffer[1] != page)
> +	/* Sanity check that we at least got the header */
> +	if (resid > len - 4)
>  		return -EIO;
>  
>  	return 0;
> @@ -1027,7 +1028,7 @@ unsigned char *scsi_get_vpd_page(struct 
>  		return NULL;
>  
>  	/* Ask for all the pages supported by this device */
> -	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
> +	result = scsi_vpd_inquiry(sdev, buf, 0, 255 + 4);
>  	if (result)
>  		goto fail;
>  
> @@ -1042,7 +1043,7 @@ unsigned char *scsi_get_vpd_page(struct 
>  	goto fail;
>  
>   found:
> -	result = scsi_vpd_inquiry(sdev, buf, page, 255);
> +	result = scsi_vpd_inquiry(sdev, buf, page, 255 + 4);
>  	if (result)
>  		goto fail;
>  
> @@ -1056,7 +1057,7 @@ unsigned char *scsi_get_vpd_page(struct 
>  
>  	kfree(buf);
>  	buf = kmalloc(len + 4, GFP_KERNEL);
> -	result = scsi_vpd_inquiry(sdev, buf, page, len);
> +	result = scsi_vpd_inquiry(sdev, buf, page, len + 4);
>  	if (result)
>  		goto fail;

Sort of, but it's not really doing it properly.  Lets do it like this.
This should also fix the > 255 length problem older devices might have.

James

---

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2de5f3a..b6e0307 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -994,7 +994,7 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, NULL);
 	if (result)
 		return result;
 
@@ -1021,13 +1021,14 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
 {
 	int i, result;
 	unsigned int len;
-	unsigned char *buf = kmalloc(259, GFP_KERNEL);
+	const unsigned int init_vpd_len = 255;
+	unsigned char *buf = kmalloc(init_vpd_len, GFP_KERNEL);
 
 	if (!buf)
 		return NULL;
 
 	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
+	result = scsi_vpd_inquiry(sdev, buf, 0, init_vpd_len);
 	if (result)
 		goto fail;
 
@@ -1050,12 +1051,12 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
 	 * Some pages are longer than 255 bytes.  The actual length of
 	 * the page is returned in the header.
 	 */
-	len = (buf[2] << 8) | buf[3];
-	if (len <= 255)
+	len = ((buf[2] << 8) | buf[3]) + 4;
+	if (len <= init_vpd_len)
 		return buf;
 
 	kfree(buf);
-	buf = kmalloc(len + 4, GFP_KERNEL);
+	buf = kmalloc(len, GFP_KERNEL);
 	result = scsi_vpd_inquiry(sdev, buf, page, len);
 	if (result)
 		goto fail;

Re: Bugs in scsi_vpd_inquiry()

[Matthew Wilcox]

On Mon, Aug 10, 2009 at 11:32:00AM -0400, Alan Stern wrote:
> > > Is there some reason for not accounting for the 4 header bytes in the 
> > > allocation length value stored in the CDB?  Or is this simply a bug?
> > 
> > Um, we do.
> > 
> > unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> >         unsigned char *buf = kmalloc(259, GFP_KERNEL);
> >         result = scsi_vpd_inquiry(sdev, buf, 0, 255);
> >         for (i = 0; i < buf[3]; i++)
> >                 if (buf[i + 4] == page)
> >                         goto found;
> >         buf = kmalloc(len + 4, GFP_KERNEL);
> >         result = scsi_vpd_inquiry(sdev, buf, page, len);
> 
> I'm not referring to this routine; I'm talking about the code in 
> scsi_vpd_inquiry() where the CDB is set.

You could have been a little clearer in your bug report.

> > > Were you aware that SCSI-2 defines the allocation length to be a single 
> > > byte?  cmd[3] is specified as "Reserved" in the spec.  Hence the value 
> > > of "len" should be capped at 255 if sdev->scsi_level <= SCSI_2, right?
> > 
> > and 'Reserved' in SCSI-2 means:
> > 
> > "A reserved bit, field, or byte shall be set to zero, or in accordance
> > with a future extension to this standard." (7.1.1)
> 
> Sure.  But what reason could there possibly be for making a field
> non-zero when you know that the device won't be able to interpret the
> value correctly?  The fact that sdev->scsi_level == SCSI_2 means
> the device follows _this_ version of the standard, not a future 
> extension.  Or am I missing something?

Because with my misunderstanding of the allocation length to be the page
length, we wouldn't ever request more than 255 bytes until the device
said it supported a page which is more than 255 bytes long.  Since such
a device must be outside the SCSI-2 spec, it was assumed to understand
the meaning of the 'reserved' byte.  So that wasn't a bug.

Now that we need to pass 259 to get 255 bytes, that would be a bug,
so we need to take care of it.

> (And there's no reason to be rude.  I'm trying to hold a civil
> discussion.)

It didn't feel like it.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Bugs in scsi_vpd_inquiry()

[Alan Stern]

On Tue, 11 Aug 2009, James Bottomley wrote:

> Sort of, but it's not really doing it properly.  Lets do it like this.
> This should also fix the > 255 length problem older devices might have.

Do you mind including also the residue check?

Alan Stern



Index: usb-2.6/drivers/scsi/scsi.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi.c
+++ usb-2.6/drivers/scsi/scsi.c
@@ -980,7 +980,8 @@ static int scsi_vpd_inquiry(struct scsi_
 							u8 page, unsigned len)
 {
 	int result;
-	unsigned char cmd[16];
+	int resid;
+	unsigned char cmd[6];
 
 	cmd[0] = INQUIRY;
 	cmd[1] = 1;		/* EVPD */
@@ -994,12 +995,12 @@ static int scsi_vpd_inquiry(struct scsi_
 	 * all the existing users tried this hard.
 	 */
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
-				  len + 4, NULL, 30 * HZ, 3, NULL);
+				  len, NULL, 30 * HZ, 3, &resid);
 	if (result)
 		return result;
 
-	/* Sanity check that we got the page back that we asked for */
-	if (buffer[1] != page)
+	/* Sanity check that we got the header and the page we asked for */
+	if (resid > len - 4 || buffer[1] != page)
 		return -EIO;
 
 	return 0;
@@ -1021,13 +1022,14 @@ unsigned char *scsi_get_vpd_page(struct 
 {
 	int i, result;
 	unsigned int len;
-	unsigned char *buf = kmalloc(259, GFP_KERNEL);
+	const unsigned int init_vpd_len = 255;
+	unsigned char *buf = kmalloc(init_vpd_len, GFP_KERNEL);
 
 	if (!buf)
 		return NULL;
 
 	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, 255);
+	result = scsi_vpd_inquiry(sdev, buf, 0, init_vpd_len);
 	if (result)
 		goto fail;
 
@@ -1042,7 +1044,7 @@ unsigned char *scsi_get_vpd_page(struct 
 	goto fail;
 
  found:
-	result = scsi_vpd_inquiry(sdev, buf, page, 255);
+	result = scsi_vpd_inquiry(sdev, buf, page, init_vpd_len);
 	if (result)
 		goto fail;
 
@@ -1050,12 +1052,12 @@ unsigned char *scsi_get_vpd_page(struct 
 	 * Some pages are longer than 255 bytes.  The actual length of
 	 * the page is returned in the header.
 	 */
-	len = (buf[2] << 8) | buf[3];
-	if (len <= 255)
+	len = ((buf[2] << 8) | buf[3]) + 4;
+	if (len <= init_vpd_len)
 		return buf;
 
 	kfree(buf);
-	buf = kmalloc(len + 4, GFP_KERNEL);
+	buf = kmalloc(len, GFP_KERNEL);
 	result = scsi_vpd_inquiry(sdev, buf, page, len);
 	if (result)
 		goto fail;

Re: Bugs in scsi_vpd_inquiry()

[James Bottomley]

On Tue, 2009-08-11 at 12:14 -0400, Alan Stern wrote:
> On Tue, 11 Aug 2009, James Bottomley wrote:
> 
> > Sort of, but it's not really doing it properly.  Lets do it like this.
> > This should also fix the > 255 length problem older devices might have.
> 
> Do you mind including also the residue check?

But there's no point to it ... it's a Byzantine check.  The standard
says shall return as many bytes as will fit in the allocation length
(what it does with allocation length beyond data to return is
undefined).

For the USB case where a full residue and no error indicates there was
actually an error, we already have a translation.

If devices just return random data lengths and then stop, your proposed
residue check doesn't catch them anyway.  However, I'd much rather
assume a device performs to spec until proven otherwise.

James

Re: Bugs in scsi_vpd_inquiry()

[Boaz Harrosh]

On 08/11/2009 07:24 PM, James Bottomley wrote:
> On Tue, 2009-08-11 at 12:14 -0400, Alan Stern wrote:
>> On Tue, 11 Aug 2009, James Bottomley wrote:
>>
>>> Sort of, but it's not really doing it properly.  Lets do it like this.
>>> This should also fix the > 255 length problem older devices might have.
>>
>> Do you mind including also the residue check?
> 
> But there's no point to it ... it's a Byzantine check.  The standard
> says shall return as many bytes as will fit in the allocation length
> (what it does with allocation length beyond data to return is
> undefined).
> 
> For the USB case where a full residue and no error indicates there was
> actually an error, we already have a translation.
> 
> If devices just return random data lengths and then stop, your proposed
> residue check doesn't catch them anyway.  However, I'd much rather
> assume a device performs to spec until proven otherwise.
> 

OK so this is also the case for devices that did not error, did not return
resid, and yet did not return the page in question. Please remove that
!= page check then. ("until proven otherwise")

> James
> 
> 

Boaz

Re: Bugs in scsi_vpd_inquiry()

[James Bottomley]

On Thu, 2009-08-13 at 16:58 +0300, Boaz Harrosh wrote:
> On 08/11/2009 07:24 PM, James Bottomley wrote:
> > On Tue, 2009-08-11 at 12:14 -0400, Alan Stern wrote:
> >> On Tue, 11 Aug 2009, James Bottomley wrote:
> >>
> >>> Sort of, but it's not really doing it properly.  Lets do it like this.
> >>> This should also fix the > 255 length problem older devices might have.
> >>
> >> Do you mind including also the residue check?
> > 
> > But there's no point to it ... it's a Byzantine check.  The standard
> > says shall return as many bytes as will fit in the allocation length
> > (what it does with allocation length beyond data to return is
> > undefined).
> > 
> > For the USB case where a full residue and no error indicates there was
> > actually an error, we already have a translation.
> > 
> > If devices just return random data lengths and then stop, your proposed
> > residue check doesn't catch them anyway.  However, I'd much rather
> > assume a device performs to spec until proven otherwise.
> > 
> 
> OK so this is also the case for devices that did not error, did not return
> resid, and yet did not return the page in question. Please remove that
> != page check then. ("until proven otherwise")

The usual presumption is that if a check is there, there's a reason for
it.  There are actually several devices that only return the one VPD
page they're programmed for whatever you happen to ask for ... that's
why the check is there.

James