PATCH libata-2.6 4/5] Prevent the device from overrunning the buffer in __atapi_pio_bytes()

PATCH libata-2.6 4/5] Prevent the device from overrunning the buffer in __atapi_pio_bytes()

[Albert Lee]

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

Hi Jeff,

Problem:
   Some bad behaved CD-ROM drives will return more data than ask to.
(I have such CD-RW drive and it crashed the kernel.)

Changes:
- Add additional check in __atapi_pio_bytes() to prevent the device from overrunning the buffer.

Attached please find the patch against the libata-2.6 tree for your review. Thanks.

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---------------------------------------
--- libata-2.6-reorder/drivers/scsi/libata-core.c	2005-03-18 12:56:44.000000000 +0800
+++ libata-2.6-extrabytes/drivers/scsi/libata-core.c	2005-03-18 14:00:34.000000000 +0800
@@ -2338,6 +2338,14 @@
  		ap->pio_task_state = PIO_ST_LAST;

  next_sg:
+	/* check whether qc->sg is full */
+	if (unlikely(qc->cursg >= qc->n_elem)) {
+		printk(KERN_WARNING "ata%u: qc->sg full, %u bytes trailing\n", ap->id, bytes);
+
+		ap->pio_task_state = PIO_ST_ERR;
+		return;
+	}
+
  	sg = &qc->sg[qc->cursg];

  	page = sg->page;


[-- Attachment #2: extrabytes.diff --]
[-- Type: text/plain, Size: 512 bytes --]

--- libata-2.6-reorder/drivers/scsi/libata-core.c	2005-03-18 12:56:44.000000000 +0800
+++ libata-2.6-extrabytes/drivers/scsi/libata-core.c	2005-03-18 14:00:34.000000000 +0800
@@ -2338,6 +2338,14 @@
 		ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:
+	/* check whether qc->sg is full */
+	if (unlikely(qc->cursg >= qc->n_elem)) {
+		printk(KERN_WARNING "ata%u: qc->sg full, %u bytes trailing\n", ap->id, bytes);
+
+		ap->pio_task_state = PIO_ST_ERR;
+		return;
+	}
+
 	sg = &qc->sg[qc->cursg];
 
 	page = sg->page;

Re: PATCH libata-2.6 4/5] Prevent the device from overrunning the buffer in __atapi_pio_bytes()

[Bartlomiej Zolnierkiewicz]

On Fri, 18 Mar 2005 16:00:36 +0800, Albert Lee <albertcc@tw.ibm.com> wrote:
> Hi Jeff,
> 
> Problem:
>    Some bad behaved CD-ROM drives will return more data than ask to.
> (I have such CD-RW drive and it crashed the kernel.)

These devices are compliant with original ATAPI spec.

Such condition shouldn't be treated as an error
- extra data should be read and dumped.

> Changes:
> - Add additional check in __atapi_pio_bytes() to prevent the device from overrunning the buffer.
> 
> Attached please find the patch against the libata-2.6 tree for your review. Thanks.
> 
> Albert
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---------------------------------------
> --- libata-2.6-reorder/drivers/scsi/libata-core.c       2005-03-18 12:56:44.000000000 +0800
> +++ libata-2.6-extrabytes/drivers/scsi/libata-core.c    2005-03-18 14:00:34.000000000 +0800
> @@ -2338,6 +2338,14 @@
>                 ap->pio_task_state = PIO_ST_LAST;
> 
>   next_sg:
> +       /* check whether qc->sg is full */
> +       if (unlikely(qc->cursg >= qc->n_elem)) {
> +               printk(KERN_WARNING "ata%u: qc->sg full, %u bytes trailing\n", ap->id, bytes);
> +
> +               ap->pio_task_state = PIO_ST_ERR;
> +               return;
> +       }
> +
>         sg = &qc->sg[qc->cursg];
> 
>         page = sg->page;
> 
> 
> --- libata-2.6-reorder/drivers/scsi/libata-core.c       2005-03-18 12:56:44.000000000 +0800
> +++ libata-2.6-extrabytes/drivers/scsi/libata-core.c    2005-03-18 14:00:34.000000000 +0800
> @@ -2338,6 +2338,14 @@
>                 ap->pio_task_state = PIO_ST_LAST;
> 
>  next_sg:
> +       /* check whether qc->sg is full */
> +       if (unlikely(qc->cursg >= qc->n_elem)) {
> +               printk(KERN_WARNING "ata%u: qc->sg full, %u bytes trailing\n", ap->id, bytes);
> +
> +               ap->pio_task_state = PIO_ST_ERR;
> +               return;
> +       }
> +
>         sg = &qc->sg[qc->cursg];
> 
>         page = sg->page;
> 
> 
>

Re: PATCH libata-2.6 4/5] Prevent the device from overrunning the buffer in __atapi_pio_bytes()

[Albert Lee]

 >
 > Bartlomiej Zolnierkiewicz wrote:
>>
>>Problem:
>>   Some bad behaved CD-ROM drives will return more data than ask to.
>>(I have such CD-RW drive and it crashed the kernel.)
> 
> 
> These devices are compliant with original ATAPI spec.
> 
> Such condition shouldn't be treated as an error
> - extra data should be read and dumped.
> 
> 
Hi Bart,

For read, we can read and discard the extra data from the device.
For write, the device is asking for more data than we have.
Should we supply some dummy data to the device? Or we just stop and return it as error?


Albert

Re: PATCH libata-2.6 4/5] Prevent the device from overrunning the buffer in __atapi_pio_bytes()

[Jeff Garzik]

Albert Lee wrote:
>  >
>  > Bartlomiej Zolnierkiewicz wrote:
> 
>>>
>>> Problem:
>>>   Some bad behaved CD-ROM drives will return more data than ask to.
>>> (I have such CD-RW drive and it crashed the kernel.)
>>
>>
>>
>> These devices are compliant with original ATAPI spec.
>>
>> Such condition shouldn't be treated as an error
>> - extra data should be read and dumped.
>>
>>
> Hi Bart,
> 
> For read, we can read and discard the extra data from the device.
> For write, the device is asking for more data than we have.
> Should we supply some dummy data to the device? Or we just stop and 
> return it as error?

For write, we need to pad.  For DMA, we need to add additional zeroes, 
until the DMA transfer ends on a dword boundary.  For PIO, we must at 
least pad to a word boundary.

	Jeff

Re: PATCH libata-2.6 4/5] Prevent the device from overrunning the buffer in __atapi_pio_bytes()

[Albert Lee]

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



Jeff Garzik wrote:
> Albert Lee wrote:
> 
>>  >
>>  > Bartlomiej Zolnierkiewicz wrote:
>>
>>>>
>>>> Problem:
>>>>   Some bad behaved CD-ROM drives will return more data than ask to.
>>>> (I have such CD-RW drive and it crashed the kernel.)
>>>
>>>
>>> These devices are compliant with original ATAPI spec.
>>>
>>> Such condition shouldn't be treated as an error
>>> - extra data should be read and dumped.
>>>
>>>
>> Hi Bart,
>>
>> For read, we can read and discard the extra data from the device.
>> For write, the device is asking for more data than we have.
>> Should we supply some dummy data to the device? Or we just stop and 
>> return it as error?
> 
> 
> For write, we need to pad.  For DMA, we need to add additional zeroes, 
> until the DMA transfer ends on a dword boundary.  For PIO, we must at 
> least pad to a word boundary.
> 
>     Jeff
> 


Attached please find the revised patch for your review.

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---------------------------------------
--- libata-2.6-reorder/drivers/scsi/libata-core.c	2005-03-18 12:56:44.000000000 +0800
+++ libata-2.6-extrabytes/drivers/scsi/libata-core.c	2005-03-29 20:23:24.000000000 +0800
@@ -2338,6 +2338,23 @@
  		ap->pio_task_state = PIO_ST_LAST;

  next_sg:
+	/* check whether qc->sg is full */
+	if (unlikely(qc->cursg >= qc->n_elem)) {
+		unsigned char pad_buf[2];
+		unsigned int words = (bytes+1) >> 1; /* pad to word boundary */
+		unsigned int i;
+
+		DPRINTK("ata%u: padding %u bytes\n", ap->id, bytes);
+
+		memset(&pad_buf, 0, sizeof(pad_buf));
+		for (i = 0; i < words; i++) {
+			ata_data_xfer(ap, pad_buf, sizeof(pad_buf), do_write);
+		}
+
+		ap->pio_task_state = PIO_ST_LAST;
+		return;
+	}
+
  	sg = &qc->sg[qc->cursg];

  	page = sg->page;


[-- Attachment #2: extrabytes.diff --]
[-- Type: text/plain, Size: 744 bytes --]

--- libata-2.6-reorder/drivers/scsi/libata-core.c	2005-03-18 12:56:44.000000000 +0800
+++ libata-2.6-extrabytes/drivers/scsi/libata-core.c	2005-03-29 20:23:24.000000000 +0800
@@ -2338,6 +2338,23 @@
 		ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:
+	/* check whether qc->sg is full */
+	if (unlikely(qc->cursg >= qc->n_elem)) {
+		unsigned char pad_buf[2];
+		unsigned int words = (bytes+1) >> 1; /* pad to word boundary */
+		unsigned int i;
+
+		DPRINTK("ata%u: padding %u bytes\n", ap->id, bytes);
+
+		memset(&pad_buf, 0, sizeof(pad_buf));
+		for (i = 0; i < words; i++) {
+			ata_data_xfer(ap, pad_buf, sizeof(pad_buf), do_write);
+		}
+
+		ap->pio_task_state = PIO_ST_LAST;
+		return;
+	}
+
 	sg = &qc->sg[qc->cursg];
 
 	page = sg->page;