[RFC] Sanitize INQUIRY vendor, product and revision strings

[RFC] Sanitize INQUIRY vendor, product and revision strings

[Alan Stern]

Does anyone object to the patch below?  It would help at least one person; 
I'm concerned that it might cause trouble somewhere else.

The idea is to replace all non-graphic characters in the INQUIRY ASCII 
strings with ' '.  According to the SCSI spec these fields shouldn't 
contain any non-graphic characters to begin with, but some non-compliant 
devices have NUL characters and possibly others.

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
@@ -148,27 +148,19 @@ static void scsi_unlock_floptical(struct
 static void print_inquiry(unsigned char *inq_result)
 {
 	int i;
+	int n = inq_result[4] + 5;
 
 	printk(KERN_NOTICE "  Vendor: ");
 	for (i = 8; i < 16; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
+		printk("%c", (i < n ? inq_result[i] : ' '));
 
 	printk("  Model: ");
 	for (i = 16; i < 32; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
+		printk("%c", (i < n ? inq_result[i] : ' '));
 
 	printk("  Rev: ");
 	for (i = 32; i < 36; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
+		printk("%c", (i < n ? inq_result[i] : ' '));
 
 	printk("\n");
 
@@ -463,13 +455,14 @@ void scsi_target_reap(struct scsi_target
  *     INQUIRY data is in @inq_result; the scsi_level and INQUIRY length
  *     are copied to the scsi_device any flags value is stored in *@bflags.
  **/
-static int scsi_probe_lun(struct scsi_device *sdev, char *inq_result,
+static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			  int result_len, int *bflags)
 {
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
 	int response_len = 0;
 	int pass, count, result;
+	int i;
 	struct scsi_sense_hdr sshdr;
 
 	*bflags = 0;
@@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
 		if (response_len > 255)
 			response_len = first_inquiry_len;	/* sanity */
 
+		/* Sanitize the Vendor, Product, and Revision fields. */
+		for (i = 8; i < 36; ++i) {
+			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
+				inq_result[i] = ' ';
+		}
+
 		/*
 		 * Get any flags for this device.
 		 *
@@ -628,7 +627,8 @@ static int scsi_probe_lun(struct scsi_de
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a scsi_device
  *     SCSI_SCAN_LUN_PRESENT: a new scsi_device was allocated and initialized
  **/
-static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
+static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
+		int *bflags)
 {
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,

Re: [RFC] Sanitize INQUIRY vendor, product and revision strings

[Matthew Wilcox]

On Mon, Aug 14, 2006 at 12:34:56PM -0400, Alan Stern wrote:
> Does anyone object to the patch below?  It would help at least one person; 
> I'm concerned that it might cause trouble somewhere else.

> @@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
>  		if (response_len > 255)
>  			response_len = first_inquiry_len;	/* sanity */
>  
> +		/* Sanitize the Vendor, Product, and Revision fields. */
> +		for (i = 8; i < 36; ++i) {
> +			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
> +				inq_result[i] = ' ';
> +		}
> +
>  		/*

If we have one non-printable character, should we not overwrite all
subsequent characters with spaces?  I'm concerned that they may
NUL-terminate a string, then the remnants be garbage.  So in other
words, I'd like to see something like:

void scsi_inquiry_sanitise(unsigned char *inquiry, int start, int end)
{
	int i, unprintable = 0;
	for (i = start; i < end; i++) {
		unprintable |= (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
		if (unprintable)
			inq_result[i] = ' ';
	}
}

and then call it:

	scsi_inquiry_sanitise(inq_result, 8, 16);
	scsi_inquiry_sanitise(inq_result, 16, 32);
	scsi_inquiry_sanitise(inq_result, 32, 36);

This of course begs the questions:
 - Do we want to mangle the inquiry return like this?
 - Should we simply copy out the strings from the raw inquiry result, and
   fix them at copy time
 - What about someone who just put a tab in there and now gets
   everything overwritten with spaces

That last one could be fixed with a more complex function ...

void scsi_inquiry_sanitise(unsigned char *inquiry, int start, int end)
{
	int i, terminated = 0;
	for (i = start; i < end; i++) {
		terminated |= (inq_result[i] == 0)
		if (terminated || inq_result[i] < 0x20 || inq_result[i] > 0x7e)
			inq_result[i] = ' ';
	}
}

Re: [RFC] Sanitize INQUIRY vendor, product and revision strings

[Philip R. Auld]

Hi,

Rumor has it that on Mon, Aug 14, 2006 at 11:07:31AM -0600 Matthew Wilcox said:
> On Mon, Aug 14, 2006 at 12:34:56PM -0400, Alan Stern wrote:
> > Does anyone object to the patch below?  It would help at least one person; 
> > I'm concerned that it might cause trouble somewhere else.
> 
> > @@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
> >  		if (response_len > 255)
> >  			response_len = first_inquiry_len;	/* sanity */
> >  
> > +		/* Sanitize the Vendor, Product, and Revision fields. */
> > +		for (i = 8; i < 36; ++i) {
> > +			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
> > +				inq_result[i] = ' ';
> > +		}
> > +
> >  		/*
> 
> If we have one non-printable character, should we not overwrite all
> subsequent characters with spaces?  I'm concerned that they may
> NUL-terminate a string, then the remnants be garbage.  

I wouldn't make an exception for NUL. Trying to guess that the device 
vendor really meant to NUL-terminate does not seem right to me. 
Plus it will hide out of spec devices more than "spacing" unprintable 
characters.

...

> This of course begs the questions:
>  - Do we want to mangle the inquiry return like this?
>  - Should we simply copy out the strings from the raw inquiry result, and
>    fix them at copy time

This is probably better, at the expense of some space though. 


FWIW,


Phil



-- 
Philip R. Auld, Ph.D.  	        	       Egenera, Inc.    
Software Architect                            165 Forest St.
(508) 858-2628                            Marlboro, MA 01752

Re: [RFC] Sanitize INQUIRY vendor, product and revision strings

[Stefan Richter]

>> On Mon, Aug 14, 2006 at 12:34:56PM -0400, Alan Stern wrote:
>>> Does anyone object to the patch below?  It would help at least one person; 
>>> I'm concerned that it might cause trouble somewhere else.
>>> @@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
>>>  		if (response_len > 255)
>>>  			response_len = first_inquiry_len;	/* sanity */
>>>  
>>> +		/* Sanitize the Vendor, Product, and Revision fields. */
>>> +		for (i = 8; i < 36; ++i) {
>>> +			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
>>> +				inq_result[i] = ' ';
>>> +		}
>>> +
>>>  		/*

This does affect all subsequent usage of the inquiry data, doesn't it? I.e.
  - comparison against blacklist entries for device flags,
  - readout in dmesg,
  - contents of sysfs attributes,
  - text in procfs?
-- 
Stefan Richter
-=====-=-==- =--- -===-
http://arcgraph.de/sr/

Re: [RFC] Sanitize INQUIRY vendor, product and revision strings

[Alan Stern]

On Mon, 14 Aug 2006, Stefan Richter wrote:

> >> On Mon, Aug 14, 2006 at 12:34:56PM -0400, Alan Stern wrote:
> >>> Does anyone object to the patch below?  It would help at least one person; 
> >>> I'm concerned that it might cause trouble somewhere else.
> >>> @@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
> >>>  		if (response_len > 255)
> >>>  			response_len = first_inquiry_len;	/* sanity */
> >>>  
> >>> +		/* Sanitize the Vendor, Product, and Revision fields. */
> >>> +		for (i = 8; i < 36; ++i) {
> >>> +			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
> >>> +				inq_result[i] = ' ';
> >>> +		}
> >>> +
> >>>  		/*
> 
> This does affect all subsequent usage of the inquiry data, doesn't it? I.e.
>   - comparison against blacklist entries for device flags,
>   - readout in dmesg,
>   - contents of sysfs attributes,
>   - text in procfs?

Indeed it will.  In fact, the main purpose of the patch was to change the
comparison against blacklist entries for device flags.  The device in
question returns a NUL character in the Vendor string, making it
impossible for a dynamic blacklist entry (which is always padded to 8
characters) to match.

For the blacklist, I think really is what we want.  For everything else, I
think the change should be harmless -- expect perhaps that it replaces
NULs with spaces.  This will cause text beyond the NUL to show up where it
did not before; if it happens to contain garbage then the users will see
that garbage.  At least the garbage won't include any weird unprintable or
non-ASCII characters.

Remember, the standard says that these fields must not contain any 
non-graphic characters and must be padded with spaces.  Only non-compliant 
devices would be affected, and for the most part the effect will be 
minimal.

In fact, I can't think up a scenario where this would cause unfixable 
damage.  Of course that doesn't mean such scenarios don't exist.  :-)

Alan Stern

Re: [RFC] Sanitize INQUIRY vendor, product and revision strings

[Stefan Richter]

Alan Stern wrote:
...
> In fact, the main purpose of the patch was to change the
> comparison against blacklist entries for device flags.
...
> For everything else, I think the change should be harmless
...

Looks good to me. (However we in the SBP-2 corner rarely deal with the 
inquiry's vendor/ model/ revision data, so I have no strong opinion.)
-- 
Stefan Richter
-=====-=-==- =--- -====
http://arcgraph.de/sr/