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

[Matthew Wilcox <matthew@wil.cx>]

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] = ' ';
	}
}