Re: [PATCH] Oops in usb-storage.c

[Matthew Dharm <mdharm-kernel@one-eyed-alien.net>]

[-- Attachment #1: Type: text/plain, Size: 6720 bytes --]

This looks better... but you have a dangerous memset() in an else clause --
the request buffer pointer could be pointing to a scatter-gather list, so
you can't just memset on it.

Matt

On Wed, Oct 17, 2001 at 12:42:49PM +0200, Jan Niehusmann wrote:
> On Tue, Oct 16, 2001 at 11:24:52PM -0700, Matthew Dharm wrote:
> > Technically, as long as the system believes that the target exists (i.e.
> > you haven't unloaded your SCSI driver module), the target is required to
> > respond to an INQUIRY command.  So, if you boot with the scanner on, and
> > then turn it off, you've got a problem.
> 
> Ok. I looked at the SCSI-2 standard and found a possibly sensible answer
> for such an INQUIRY to a disconnected device.
> First, there is a special peripheral qualifier for disconnected
> physical devices:
> 
> 001b	The target is capable of supporting the specified peripheral
> 	device type on this logical unit;  however, the physical device 
> 	is not currently connected to this logical unit.   
> 
> Second, the devices is not required to give a complete answer:
> 
>   NOTES
>   65 The INQUIRY command is typically used by the initiator after a reset
>   or power-up condition to determine the device types for system
>   configuration. To minimize delays after a reset or power-up condition,
>   the standard INQUIRY data should be available without incurring any
>   media access delays. If the target does store some of the INQUIRY data
>   on the device, it may return zeros or ASCII spaces (20h) in those fields
>   until the data is available from the device.
> 
> While this permission to set some fields to zero is included in the
> standard for other purposes, it makes clear that you must expect to
> get incomplete answers from an INQUIRY command.
> 
> So, what about the following patch?
> 
> Jan
> 
> 
> 	
> --- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig	Mon Oct  1 12:15:29 2001
> +++ linux-2.4.12-ac3/drivers/usb/storage/usb.c	Wed Oct 17 12:33:40 2001
> @@ -262,16 +262,28 @@
>  	if (data_len<36) // You lose.
>  		return;
>  
> -	memcpy(data+8, us->unusual_dev->vendorName, 
> -		strlen(us->unusual_dev->vendorName) > 8 ? 8 :
> -		strlen(us->unusual_dev->vendorName));
> -	memcpy(data+16, us->unusual_dev->productName, 
> -		strlen(us->unusual_dev->productName) > 16 ? 16 :
> -		strlen(us->unusual_dev->productName));
> -	data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
> -	data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
> -	data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
> -	data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
> +	if(data[0]&0x20) { /* USB device currently not connected. Return
> +			      peripheral qualifier 001b ("...however, the
> +			      physical device is not currently connected
> +			      to this logical unit") and leave vendor and
> +			      product identification empty. ("If the target
> +			      does store some of the INQUIRY data on the
> +			      device, it may return zeros or ASCII spaces 
> +			      (20h) in those fields until the data is
> +			      available from the device."). */
> +		memset(data+8,0,28);
> +	} else {
> +		memcpy(data+8, us->unusual_dev->vendorName, 
> +			strlen(us->unusual_dev->vendorName) > 8 ? 8 :
> +			strlen(us->unusual_dev->vendorName));
> +		memcpy(data+16, us->unusual_dev->productName, 
> +			strlen(us->unusual_dev->productName) > 16 ? 16 :
> +			strlen(us->unusual_dev->productName));
> +		data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
> +		data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
> +		data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
> +		data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
> +	}
>  
>  	if (us->srb->use_sg) {
>  		sg = (struct scatterlist *)us->srb->request_buffer;
> @@ -389,24 +401,6 @@
>  				break;
>  			}
>  
> -			/* Handle those devices which need us to fake their
> -			 * inquiry data */
> -			if ((us->srb->cmnd[0] == INQUIRY) &&
> -			    (us->flags & US_FL_FIX_INQUIRY)) {
> -			    	unsigned char data_ptr[36] = {
> -				    0x00, 0x80, 0x02, 0x02,
> -				    0x1F, 0x00, 0x00, 0x00};
> -
> -			    	US_DEBUGP("Faking INQUIRY command\n");
> -				fill_inquiry_response(us, data_ptr, 36);
> -				us->srb->result = GOOD << 1;
> -
> -				set_current_state(TASK_INTERRUPTIBLE);
> -				us->srb->scsi_done(us->srb);
> -				us->srb = NULL;
> -				break;
> -			}
> -
>  			/* lock the device pointers */
>  			down(&(us->dev_semaphore));
>  
> @@ -422,16 +416,38 @@
>  					       usb_stor_sense_notready, 
>  					       sizeof(usb_stor_sense_notready));
>  					us->srb->result = GOOD << 1;
> +				} else if(us->srb->cmnd[0] == INQUIRY) {
> +					unsigned char data_ptr[36] = {
> +					    0x20, 0x80, 0x02, 0x02,
> +					    0x1F, 0x00, 0x00, 0x00};
> +					US_DEBUGP("Faking INQUIRY command for disconnected device\n");
> +					fill_inquiry_response(us, data_ptr, 36);
> +					us->srb->result = GOOD << 1;
>  				} else {
> +					memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
>  					memcpy(us->srb->sense_buffer, 
>  					       usb_stor_sense_notready, 
>  					       sizeof(usb_stor_sense_notready));
>  					us->srb->result = CHECK_CONDITION << 1;
>  				}
>  			} else { /* !us->pusb_dev */
> -				/* we've got a command, let's do it! */
> -				US_DEBUG(usb_stor_show_command(us->srb));
> -				us->proto_handler(us->srb, us);
> +
> +				/* Handle those devices which need us to fake 
> +				 * their inquiry data */
> +				if ((us->srb->cmnd[0] == INQUIRY) &&
> +				    (us->flags & US_FL_FIX_INQUIRY)) {
> +					unsigned char data_ptr[36] = {
> +					    0x00, 0x80, 0x02, 0x02,
> +					    0x1F, 0x00, 0x00, 0x00};
> +
> +					US_DEBUGP("Faking INQUIRY command\n");
> +					fill_inquiry_response(us, data_ptr, 36);
> +					us->srb->result = GOOD << 1;
> +				} else {
> +					/* we've got a command, let's do it! */
> +					US_DEBUG(usb_stor_show_command(us->srb));
> +					us->proto_handler(us->srb, us);
> +				}
>  			}
>  
>  			/* unlock the device pointers */
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Dudes! May the Open Source be with you.
					-- Eric S. Raymond
User Friendly, 12/3/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]