Re: [linux-usb-devel] 2.6.24: NULL scatter-gather pointer in usb_storage:usb_stor_access_xfer_buf?
On Thu, 31 Jan 2008 11:27:39 +0200 Boaz Harrosh <[EMAIL PROTECTED]> wrote: > Please check the below patch. > > one thing that I can see is that the isd200 does an INQUARY transfer > of sizeof(struct inquiry_data) which is 96 bytes, when scsi_scan.c > sends an INQUARY with 36 bytes buffer. So we have an underflow in > usb_stor_access_xfer_buf(). > > The below patch will only check my theory. I will send a proper fix > later, please confirm that this fixes it. > > What kills me is that this condition has existed before my patch, I'll > try to see why it is triggered now I applied this patch to 2.6.24, and it now works for me. It was crashing consistently whenever I'd plug this device in, now it goes through successfully: [24775.788039] usb 3-2: new full speed USB device using uhci_hcd and address 3 [24775.939275] usb 3-2: configuration #1 chosen from 1 choice [24776.084409] usbcore: registered new interface driver libusual [24776.103604] Initializing USB Mass Storage driver... [24776.213916] scsi3 : SCSI emulation for USB Mass Storage devices [24776.214366] usbcore: registered new interface driver usb-storage [24776.214377] USB Mass Storage support registered. [24776.215604] usb-storage: device found at 3 [24776.215724] usb-storage: waiting for device to settle before scanning [24778.78] scsi 3:0:0:0: Direct-Access SAMSUNG HM120JC YL10 PQ: 0 ANSI: 0 [24778.333715] sd 3:0:0:0: [sdb] 234441648 512-byte hardware sectors (120034 MB) [24778.333841] sd 3:0:0:0: [sdb] Write Protect is off [24778.333848] sd 3:0:0:0: [sdb] Mode Sense: 00 00 00 00 [24778.333853] sd 3:0:0:0: [sdb] Assuming drive cache: write through [24778.334196] sd 3:0:0:0: [sdb] 234441648 512-byte hardware sectors (120034 MB) [24778.334396] sd 3:0:0:0: [sdb] Write Protect is off [24778.334403] sd 3:0:0:0: [sdb] Mode Sense: 00 00 00 00 [24778.334408] sd 3:0:0:0: [sdb] Assuming drive cache: write through [24778.334414] sdb: sdb1 [24778.824103] sd 3:0:0:0: [sdb] Attached SCSI disk [24778.824210] sd 3:0:0:0: Attached scsi generic sg1 type 0 [24778.825119] usb-storage: device scan complete I'm happy to test further patches. Let me know if you need more testing. Do you still want me to try out the scsi-misc branch? Mark > > --- > drivers/usb/storage/protocol.c |6 ++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/storage/protocol.c > b/drivers/usb/storage/protocol.c index a41ce21..d0ff1f6 100644 > --- a/drivers/usb/storage/protocol.c > +++ b/drivers/usb/storage/protocol.c > @@ -229,6 +229,12 @@ void usb_stor_set_xfer_buf(unsigned char *buffer, > unsigned int offset = 0; > struct scatterlist *sg = NULL; > > + BUG_ON(!scsi_sglist(srb)); > + > + if(buflen > scsi_bufflen(srb)) > + buflen = scsi_bufflen(srb); > + /*FIXME: should we set an underflow condition here*/ > + > usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset, > TO_XFER_BUF); > if (buflen < scsi_bufflen(srb)) > - To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24] bugfix for an overflow condition in usb storage & isd200.c
On Thu, 31 Jan 2008 21:37:13 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>
> Mark - This is for you on top of vanila v2.6.24 kernel from Linus.
Sorry I didn't get back to you sooner. This fixes the issue I was
having.
> ---
>
> scsi_scan is issuing a 36-byte INQUIRY request to llds. isd200 would
> volunteer 96 bytes of INQUIRY. This caused an overflow condition in
> protocol.c usb_stor_access_xfer_buf(). So first fix is to
> usb_stor_access_xfer_buf() to properly handle overflow/underflow
> conditions. Then usb_stor_set_xfer_buf() should report this condition
> as a negative resid. Should we also set cmnd->status in the overflow
> condition?
>
> Then also isd200.c is fixed to only return the type of INQUIRY &&
> SENSE the upper layer asked for.
>
> Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Tested-by: Mark Glines <[EMAIL PROTECTED]>
> ---
> drivers/usb/storage/isd200.c |7 +--
> drivers/usb/storage/protocol.c | 13 +
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/storage/isd200.c
> b/drivers/usb/storage/isd200.c index 49ba6c0..8186e93 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -1238,6 +1238,7 @@ static int isd200_scsi_to_ata(struct scsi_cmnd
> *srb, struct us_data *us, unsigned long lba;
> unsigned long blockCount;
> unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> + unsigned xfer_len;
>
> memset(ataCdb, 0, sizeof(union ata_cdb));
>
> @@ -1247,8 +1248,9 @@ static int isd200_scsi_to_ata(struct scsi_cmnd
> *srb, struct us_data *us, US_DEBUGP(" ATA OUT - INQUIRY\n");
>
> /* copy InquiryData */
> + xfer_len = min(sizeof(info->InquiryData),
> scsi_bufflen(srb)); usb_stor_set_xfer_buf((unsigned char *)
> &info->InquiryData,
> - sizeof(info->InquiryData), srb);
> + xfer_len, srb);
> srb->result = SAM_STAT_GOOD;
> sendToTransport = 0;
> break;
> @@ -1257,7 +1259,8 @@ static int isd200_scsi_to_ata(struct scsi_cmnd
> *srb, struct us_data *us, US_DEBUGP(" ATA OUT -
> SCSIOP_MODE_SENSE\n");
> /* Initialize the return buffer */
> - usb_stor_set_xfer_buf(senseData, sizeof(senseData),
> srb);
> + xfer_len = min(sizeof(senseData), scsi_bufflen(srb));
> + usb_stor_set_xfer_buf(senseData, xfer_len, srb);
>
> if (info->DeviceFlags & DF_MEDIA_STATUS_ENABLED)
> {
> diff --git a/drivers/usb/storage/protocol.c
> b/drivers/usb/storage/protocol.c index 889622b..038a284 100644
> --- a/drivers/usb/storage/protocol.c
> +++ b/drivers/usb/storage/protocol.c
> @@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned
> char *buffer,
>* and the starting offset within the page, and
> update
>* the *offset and *index values for the next loop.
> */ cnt = 0;
> - while (cnt < buflen) {
> + while (cnt < buflen && sg) {
> struct page *page = sg_page(sg) +
> ((sg->offset + *offset) >>
> PAGE_SHIFT); unsigned int poff =
> @@ -248,9 +248,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
> {
> unsigned int offset = 0;
> struct scatterlist *sg = NULL;
> + unsigned count;
>
> - usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> + count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg,
> &offset, TO_XFER_BUF);
> - if (buflen < srb->request_bufflen)
> - srb->resid = srb->request_bufflen - buflen;
> +
> + /* Check for overflow */
> + if (buflen > scsi_bufflen(srb))
> + count = buflen;
> +
> + scsi_set_resid(srb, scsi_bufflen(srb) - count);
> }
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bugfix for an underflow condition in usb storage & isd200.c
On Fri, 8 Feb 2008 11:46:13 -0500 (EST)
Alan Stern <[EMAIL PROTECTED]> wrote:
> On Tue, 5 Feb 2008, Matthew Dharm wrote:
>
> > We both agree that the code shouldn't run off the end of the s-g
> > list.
>
> Incidentally, if people want a simple bugfix patch for 2.6.24.stable,
> this should do it. Mark, can you confirm that this patch alone fixes
> the problem?
Confirmed: works just fine.
Tested-by: Mark Glines <[EMAIL PROTECTED]>
>
> Alan Stern
>
>
>
> Index: 2.6.24/drivers/usb/storage/protocol.c
> ===
> --- 2.6.24.orig/drivers/usb/storage/protocol.c
> +++ 2.6.24/drivers/usb/storage/protocol.c
> @@ -194,7 +194,7 @@ unsigned int usb_stor_access_xfer_buf(un
>* and the starting offset within the page, and
> update
>* the *offset and *index values for the next loop.
> */ cnt = 0;
> - while (cnt < buflen) {
> + while (cnt < buflen && sg) {
> struct page *page = sg_page(sg) +
> ((sg->offset + *offset) >>
> PAGE_SHIFT); unsigned int poff =
> @@ -249,6 +249,7 @@ void usb_stor_set_xfer_buf(unsigned char
> unsigned int offset = 0;
> struct scatterlist *sg = NULL;
>
> + buflen = min(buflen, srb->request_bufflen);
> usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
> TO_XFER_BUF);
> if (buflen < srb->request_bufflen)
>
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
