[PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Alan Stern]

This patch (as810) sets the length of the INQUIRY data to a minimum of
36 bytes, even if the device claims that not all of them are valid.
Using the data sent by the device is better than allocating a short
buffer and then reading beyond the end of it, which is what we do now.

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

---

Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -575,6 +575,19 @@ static int scsi_probe_lun(struct scsi_de
 	 * short INQUIRY), an abort here prevents any further use of the
 	 * device, including spin up.
 	 *
+	 * On the whole, the best approach seems to be to assume the first
+	 * 36 bytes are valid no matter what the device says.  That's
+	 * better than copying < 36 bytes to the inquiry-result buffer
+	 * and displaying garbage for the Vendor, Product, and Revision
+	 * strings.
+	 */
+	if (sdev->inquiry_len < 36) {
+		printk(KERN_INFO "scsi scan: INQUIRY result too short (%d),"
+				" using 36\n", sdev->inquiry_len);
+		sdev->inquiry_len = 36;
+	}
+
+	/*
 	 * Related to the above issue:
 	 *
 	 * XXX Devices (disk or all?) should be sent a TEST UNIT READY,

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Patrick Mansfield]

On Thu, Oct 26, 2006 at 05:11:27PM -0400, Alan Stern wrote:
> This patch (as810) sets the length of the INQUIRY data to a minimum of
> 36 bytes, even if the device claims that not all of them are valid.
> Using the data sent by the device is better than allocating a short
> buffer and then reading beyond the end of it, which is what we do now.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> +++ usb-2.6/drivers/scsi/scsi_scan.c
> @@ -575,6 +575,19 @@ static int scsi_probe_lun(struct scsi_de
>  	 * short INQUIRY), an abort here prevents any further use of the
>  	 * device, including spin up.
>  	 *
> +	 * On the whole, the best approach seems to be to assume the first
> +	 * 36 bytes are valid no matter what the device says.  That's

The comment is confusing, as it implies the device will modify data past
the indicated length, but well behaved devices should not do that, and
with your patch should point to zero filled data.

Just comment on what its avoiding or such like:

	Modify short inquiry_len values so we don't later point at random
	values. Devices returning an incorrect value in the INQUIRY
	additional length field will point at potentially valid data for
	Vendor, Product and Revsion, while conforming devices will point
	to zero filled data.

But definitely better to use possibly valid data for broken devices or
NUL for well behaved devices rather than garbage values.

> +	 * better than copying < 36 bytes to the inquiry-result buffer
> +	 * and displaying garbage for the Vendor, Product, and Revision
> +	 * strings.
> +	 */
> +	if (sdev->inquiry_len < 36) {
> +		printk(KERN_INFO "scsi scan: INQUIRY result too short (%d),"
> +				" using 36\n", sdev->inquiry_len);
> +		sdev->inquiry_len = 36;
> +	}
> +
> +	/*

-- Patrick Mansfield

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Douglas Gilbert]

Patrick Mansfield wrote:
> On Thu, Oct 26, 2006 at 05:11:27PM -0400, Alan Stern wrote:
>> This patch (as810) sets the length of the INQUIRY data to a minimum of
>> 36 bytes, even if the device claims that not all of them are valid.
>> Using the data sent by the device is better than allocating a short
>> buffer and then reading beyond the end of it, which is what we do now.
>>
>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>>
>> ---
>>
>> Index: usb-2.6/drivers/scsi/scsi_scan.c
>> ===================================================================
>> --- usb-2.6.orig/drivers/scsi/scsi_scan.c
>> +++ usb-2.6/drivers/scsi/scsi_scan.c
>> @@ -575,6 +575,19 @@ static int scsi_probe_lun(struct scsi_de
>>  	 * short INQUIRY), an abort here prevents any further use of the
>>  	 * device, including spin up.
>>  	 *
>> +	 * On the whole, the best approach seems to be to assume the first
>> +	 * 36 bytes are valid no matter what the device says.  That's
> 
> The comment is confusing, as it implies the device will modify data past
> the indicated length, but well behaved devices should not do that, and
> with your patch should point to zero filled data.
> 
> Just comment on what its avoiding or such like:
> 
> 	Modify short inquiry_len values so we don't later point at random
> 	values. Devices returning an incorrect value in the INQUIRY
> 	additional length field will point at potentially valid data for
> 	Vendor, Product and Revsion, while conforming devices will point
> 	to zero filled data.
> 
> But definitely better to use possibly valid data for broken devices or
> NUL for well behaved devices rather than garbage values.
> 
>> +	 * better than copying < 36 bytes to the inquiry-result buffer
>> +	 * and displaying garbage for the Vendor, Product, and Revision
>> +	 * strings.
>> +	 */
>> +	if (sdev->inquiry_len < 36) {
>> +		printk(KERN_INFO "scsi scan: INQUIRY result too short (%d),"
>> +				" using 36\n", sdev->inquiry_len);
>> +		sdev->inquiry_len = 36;
>> +	}
>> +
>> +	/*

Some more information on this subject: LLDs (including
pseudo ones like usb-storage) really should set the
Scsi_Cmnd::resid field to show, in the case of an
INQUIRY, when 36 bytes were requested, less bytes
were returned.

That OS from Redmond may have a hand here as well.
Directly attached ATA disks are found in SCSI scans and
respond to SCSI INQUIRY commands with plausible vendor,
model and revision strings but the additional length
field (byte 4 in the response) is set to 0. So code
could use that as a hint to stop sending further SCSI
commands and start sending ATA commands. ATA disks
behind USB and IEEE 1394 don't appear in SCSI scans
but do appear as "Physical Drives" and do respond
to SCSI INQUIRY commands properly. Then there are
ATA disks behind a SATL and they respond to SCSI
INQUIRY commands (and most others) properly; the
easiest way to spot them is that the vendor is "ATA     ".

Doug Gilbert

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Alan Stern]

On Fri, 27 Oct 2006, Douglas Gilbert wrote:

> Some more information on this subject: LLDs (including
> pseudo ones like usb-storage) really should set the
> Scsi_Cmnd::resid field to show, in the case of an
> INQUIRY, when 36 bytes were requested, less bytes
> were returned.

Indeed.  usb-storage does do this.  However the devices addressed by this
patch do send 36 bytes of data, all apparently plausible, just as you 
say directly-attached ATA disks do.  So checking the residue wouldn't 
help.

> That OS from Redmond may have a hand here as well.
> Directly attached ATA disks are found in SCSI scans and
> respond to SCSI INQUIRY commands with plausible vendor,
> model and revision strings but the additional length
> field (byte 4 in the response) is set to 0. So code
> could use that as a hint to stop sending further SCSI
> commands and start sending ATA commands. ATA disks
> behind USB and IEEE 1394 don't appear in SCSI scans
> but do appear as "Physical Drives" and do respond
> to SCSI INQUIRY commands properly.

I don't understand this.  What do you mean, they don't appear in SCSI 
scans?  They _are_ detected by scsi_probe_lun().  But they probably don't 
show any special ATA signature in the INQUIRY data, since that data is 
generally determined by the firmware in the USB/1394 interface.

>  Then there are
> ATA disks behind a SATL and they respond to SCSI
> INQUIRY commands (and most others) properly; the
> easiest way to spot them is that the vendor is "ATA     ".
> 
> Doug Gilbert

Alan Stern

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Douglas Gilbert]

Alan Stern wrote:
> On Fri, 27 Oct 2006, Douglas Gilbert wrote:
> 
>> Some more information on this subject: LLDs (including
>> pseudo ones like usb-storage) really should set the
>> Scsi_Cmnd::resid field to show, in the case of an
>> INQUIRY, when 36 bytes were requested, less bytes
>> were returned.
> 
> Indeed.  usb-storage does do this.  However the devices addressed by this
> patch do send 36 bytes of data, all apparently plausible, just as you 
> say directly-attached ATA disks do.  So checking the residue wouldn't 
> help.
> 
>> That OS from Redmond may have a hand here as well.
>> Directly attached ATA disks are found in SCSI scans and
>> respond to SCSI INQUIRY commands with plausible vendor,
>> model and revision strings but the additional length
>> field (byte 4 in the response) is set to 0. So code
>> could use that as a hint to stop sending further SCSI
>> commands and start sending ATA commands. ATA disks
>> behind USB and IEEE 1394 don't appear in SCSI scans
>> but do appear as "Physical Drives" and do respond
>> to SCSI INQUIRY commands properly.
> 
> I don't understand this.  What do you mean, they don't appear in SCSI 
> scans?  They _are_ detected by scsi_probe_lun().  But they probably don't 
> show any special ATA signature in the INQUIRY data, since that data is 
> generally determined by the firmware in the USB/1394 interface.

I'm not talking about linux!
For a code example look at sg_scan.c in sg3_utils version
1.22 in the sg_do_wscan() function, particularly the setting
of the 'dubious_scsi' field.

We have seen this before, especially with INQUIRY responses:
the quirks of that OS go some way to explaining why we see
some devices react they way they do.

36 byte INQUIRY responses (or greater) have been mandatory
since SCSI-2 (1992) and that means the additional length
field must be >= 31 . These devices don't give a hoot about
formal standards, only one informal one counts.

Doug Gilbert

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Alan Stern]

On Fri, 27 Oct 2006, Patrick Mansfield wrote:

> On Thu, Oct 26, 2006 at 05:11:27PM -0400, Alan Stern wrote:
> > This patch (as810) sets the length of the INQUIRY data to a minimum of
> > 36 bytes, even if the device claims that not all of them are valid.
> > Using the data sent by the device is better than allocating a short
> > buffer and then reading beyond the end of it, which is what we do now.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > ---
> > 
> > Index: usb-2.6/drivers/scsi/scsi_scan.c
> > ===================================================================
> > --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> > +++ usb-2.6/drivers/scsi/scsi_scan.c
> > @@ -575,6 +575,19 @@ static int scsi_probe_lun(struct scsi_de
> >  	 * short INQUIRY), an abort here prevents any further use of the
> >  	 * device, including spin up.
> >  	 *
> > +	 * On the whole, the best approach seems to be to assume the first
> > +	 * 36 bytes are valid no matter what the device says.  That's
> 
> The comment is confusing, as it implies the device will modify data past
> the indicated length, but well behaved devices should not do that, and
> with your patch should point to zero filled data.
> 
> Just comment on what its avoiding or such like:
> 
> 	Modify short inquiry_len values so we don't later point at random
> 	values. Devices returning an incorrect value in the INQUIRY
> 	additional length field will point at potentially valid data for
> 	Vendor, Product and Revsion, while conforming devices will point
> 	to zero filled data.
> 
> But definitely better to use possibly valid data for broken devices or
> NUL for well behaved devices rather than garbage values.

In the context of the comment already there in the code, your suggested 
text would be somewhat jarring.  I'll try to re-write it in a way that 
should satisfy everybody.

BTW, isn't it conceivable that well-behaved devices could send 36 bytes,
with only the first 8 bytes marked valid and non-NULL garbage in the last
28?  The spec isn't very clear about this; it says only that the standard
INQUIRY data contains 36 required bytes.

Alan Stern

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Patrick Mansfield]

On Sat, Oct 28, 2006 at 11:33:06AM -0400, Alan Stern wrote:
> On Fri, 27 Oct 2006, Patrick Mansfield wrote:
> 
> > On Thu, Oct 26, 2006 at 05:11:27PM -0400, Alan Stern wrote:
> > > This patch (as810) sets the length of the INQUIRY data to a minimum of
> > > 36 bytes, even if the device claims that not all of them are valid.
> > > Using the data sent by the device is better than allocating a short
> > > buffer and then reading beyond the end of it, which is what we do now.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > 
> > > ---
> > > 
> > > Index: usb-2.6/drivers/scsi/scsi_scan.c
> > > ===================================================================
> > > --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> > > +++ usb-2.6/drivers/scsi/scsi_scan.c
> > > @@ -575,6 +575,19 @@ static int scsi_probe_lun(struct scsi_de
> > >  	 * short INQUIRY), an abort here prevents any further use of the
> > >  	 * device, including spin up.
> > >  	 *
> > > +	 * On the whole, the best approach seems to be to assume the first
> > > +	 * 36 bytes are valid no matter what the device says.  That's
> > 
> > The comment is confusing, as it implies the device will modify data past
> > the indicated length, but well behaved devices should not do that, and
> > with your patch should point to zero filled data.
> > 
> > Just comment on what its avoiding or such like:
> > 
> > 	Modify short inquiry_len values so we don't later point at random
> > 	values. Devices returning an incorrect value in the INQUIRY
> > 	additional length field will point at potentially valid data for
> > 	Vendor, Product and Revsion, while conforming devices will point
> > 	to zero filled data.
> > 
> > But definitely better to use possibly valid data for broken devices or
> > NUL for well behaved devices rather than garbage values.
> 
> In the context of the comment already there in the code, your suggested 
> text would be somewhat jarring.  I'll try to re-write it in a way that 
> should satisfy everybody.

OK, though not sure what you mean, we could still point at garbage data
and lookup in the devinfo list based on that garbage. And the issue with
INQUIRY potentially changing after the device is spun up still exists.

[There were even disk drives available in the past that modified the
vendor and product id after spin up, this would be confusing but not a big
problem as long as they are not needed in black/white lists, you could
still issue an INQUIRY from user space and see current values.]

> BTW, isn't it conceivable that well-behaved devices could send 36 bytes,
> with only the first 8 bytes marked valid and non-NULL garbage in the last
> 28?  The spec isn't very clear about this; it says only that the standard
> INQUIRY data contains 36 required bytes.

So the additional length would be 3, but 36 bytes are transferred
including some garbage ...

I don't know and could not find anything in the specs. Maybe Doug or
others can answer. AFAICT if the device doesn't have the data available it
should return space filled values.

I thought there was some mention in some SCSI spec of an INQUIRY of less
than 36 bytes and then mention of the handling of it, but can't find
anything (searched in SPC spc3r32.pdf, SPC 2 spc2r20.pdf, SAM sam4r04.pdf
and even MMC mmc5r01b.pdf). So I'm wrong to say conforming devices would
point to zero filled data if the INQUIRY should return at least 36 bytes.

-- Patrick Mansfield

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Alan Stern]

This patch (as810b) copies a minimum of 36 bytes of INQUIRY data,
even if the device claims that not all of them are valid.  Often badly
behaved devices put plausible data in the Vendor, Product, and Revision
strings but set the Additional Length byte to a small value.  Using
potentially valid data is certainly better than allocating a short
buffer and then reading beyond the end of it, which is what we do now.

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

---

On Fri, 27 Oct 2006, Patrick Mansfield wrote:

> The comment is confusing, as it implies the device will modify data past
> the indicated length, but well behaved devices should not do that, and  
> with your patch should point to zero filled data.
  
> But definitely better to use possibly valid data for broken devices or
> NUL for well behaved devices rather than garbage values.

This version of the patch has an improved comment.  And it's superior in
another respect too:  It doesn't alter the value of sdev->inquiry_len.

Alan Stern


Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -621,6 +621,8 @@ static int scsi_probe_lun(struct scsi_de
 static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		int *bflags)
 {
+	int copy_len;
+
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,
 	 * save just vendor/model/rev.
@@ -631,12 +633,23 @@ static int scsi_add_lun(struct scsi_devi
 	 * scanning run at their own risk, or supply a user level program
 	 * that can correctly scan.
 	 */
-	sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
+
+	/*
+	 * Copy at least 36 bytes of INQUIRY data, so that we don't
+	 * dereference unallocated memory when accessing the Vendor,
+	 * Product, and Revision strings.  Badly behaved devices may set
+	 * the INQUIRY Additional Length byte to a small value, indicating
+	 * these strings are invalid, but often they contain plausible data
+	 * nonetheless.  It doesn't matter if the device sent < 36 bytes
+	 * total, since scsi_probe_lun() initializes inq_result with 0s.
+	 */
+	copy_len = max((int) sdev->inquiry_len, 36);
+	sdev->inquiry = kmalloc(copy_len, GFP_ATOMIC);
 	if (sdev->inquiry == NULL) {
 		return SCSI_SCAN_NO_RESPONSE;
 	}
 
-	memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
+	memcpy(sdev->inquiry, inq_result, copy_len);
 	sdev->vendor = (char *) (sdev->inquiry + 8);
 	sdev->model = (char *) (sdev->inquiry + 16);
 	sdev->rev = (char *) (sdev->inquiry + 32);

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Stefan Richter]

Alan Stern wrote:
> +	int copy_len;
...
> +	copy_len = max((int) sdev->inquiry_len, 36);
> +	sdev->inquiry = kmalloc(copy_len, GFP_ATOMIC);
>  	if (sdev->inquiry == NULL) {
>  		return SCSI_SCAN_NO_RESPONSE;
>  	}
...
> +	memcpy(sdev->inquiry, inq_result, copy_len);
...

Can be written slightly more concise:

	sdev->inquiry = kmemdup(inq_result,
				max_t(size_t, sdev->inquiry_len, 36),
				GFP_ATOMIC);
	if (sdev->inquiry == NULL)
		return SCSI_SCAN_NO_RESPONSE;

This is untested. kmemdup is only available since 2.6.19-rc1.
-- 
Stefan Richter
-=====-=-==- =-=- ====-
http://arcgraph.de/sr/

Re: [PATCH] SCSI core: always store >= 36 bytes of INQUIRY data

[Alan Stern]

This patch (as810c) copies a minimum of 36 bytes of INQUIRY data,
even if the device claims that not all of them are valid.  Often badly
behaved devices put plausible data in the Vendor, Product, and Revision
strings but set the Additional Length byte to a small value.  Using
potentially valid data is certainly better than allocating a short
buffer and then reading beyond the end of it, which is what we do now.

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

---

On Mon, 30 Oct 2006, Stefan Richter wrote:

> > +   sdev->inquiry = kmalloc(copy_len, GFP_ATOMIC);

> ...
> > +   memcpy(sdev->inquiry, inq_result, copy_len);
> ...
>    
> Can be written slightly more concise:
> 
>       sdev->inquiry = kmemdup(inq_result,
>                               max_t(size_t, sdev->inquiry_len, 36),
>                               GFP_ATOMIC);
>       if (sdev->inquiry == NULL)
>               return SCSI_SCAN_NO_RESPONSE;
>               
> This is untested. kmemdup is only available since 2.6.19-rc1.

It seems to work, so I hereby submit it.  Thanks.

Alan Stern


Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -631,12 +631,22 @@ static int scsi_add_lun(struct scsi_devi
 	 * scanning run at their own risk, or supply a user level program
 	 * that can correctly scan.
 	 */
-	sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
-	if (sdev->inquiry == NULL) {
+
+	/*
+	 * Copy at least 36 bytes of INQUIRY data, so that we don't
+	 * dereference unallocated memory when accessing the Vendor,
+	 * Product, and Revision strings.  Badly behaved devices may set
+	 * the INQUIRY Additional Length byte to a small value, indicating
+	 * these strings are invalid, but often they contain plausible data
+	 * nonetheless.  It doesn't matter if the device sent < 36 bytes
+	 * total, since scsi_probe_lun() initializes inq_result with 0s.
+	 */
+	sdev->inquiry = kmemdup(inq_result,
+				max_t(size_t, sdev->inquiry_len, 36),
+				GFP_ATOMIC);
+	if (sdev->inquiry == NULL)
 		return SCSI_SCAN_NO_RESPONSE;
-	}
 
-	memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
 	sdev->vendor = (char *) (sdev->inquiry + 8);
 	sdev->model = (char *) (sdev->inquiry + 16);
 	sdev->rev = (char *) (sdev->inquiry + 32);