[Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

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

The current implementation of GET_CONFIGURATION in the ATAPI stack of 
qemu replies a different length depending on the buffer, the data should 
be written into.

On the other hand the SCSI spec defines that length information has to 
be consistent and independent of return buffer lengths.

This patch makes the ATAPI emulation behave according to the spec and 
fixes the Darwin DVD driver.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Kevin Wolf <kwolf@suse.de>




[-- Attachment #2: atapi.patch --]
[-- Type: text/x-patch, Size: 1229 bytes --]

diff --git a/qemu/hw/ide.c b/qemu/hw/ide.c
index a2a8299..6e585e1 100644
--- a/qemu/hw/ide.c
+++ b/qemu/hw/ide.c
@@ -1717,6 +1717,7 @@ static void ide_atapi_cmd(IDEState *s)
     case GPCMD_GET_CONFIGURATION:
         {
             uint32_t len;
+            uint8_t index = 0;
 
             /* only feature 0 is supported */
             if (packet[2] != 0 || packet[3] != 0) {
@@ -1753,14 +1754,11 @@ static void ide_atapi_cmd(IDEState *s)
             }
 
             len = 8; /* header completed */
-            if (max_len > len) {
-                uint8_t index = 0;
 
-                buf[10] = 0x02 | 0x01; /* persistent and current */
-                len += 4; /* header */
-                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
-                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
-            }
+            buf[10] = 0x02 | 0x01; /* persistent and current */
+            len += 4; /* header */
+            len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
+            len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
             cpu_to_ube32(buf, len - 4); /* data length */
 
             ide_atapi_cmd_reply(s, len, max_len);

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

Is there anything particularly wrong with this patch? Does it break  
any guest OSs I am not aware of?
If not, please include it as it's a pretty obvious bug.

Alex

On May 21, 2008, at 4:34 PM, Alexander Graf wrote:

> The current implementation of GET_CONFIGURATION in the ATAPI stack  
> of qemu replies a different length depending on the buffer, the data  
> should be written into.
>
> On the other hand the SCSI spec defines that length information has  
> to be consistent and independent of return buffer lengths.
>
> This patch makes the ATAPI emulation behave according to the spec  
> and fixes the Darwin DVD driver.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Kevin Wolf <kwolf@suse.de>
>
>
>
> diff --git a/qemu/hw/ide.c b/qemu/hw/ide.c
> index a2a8299..6e585e1 100644
> --- a/qemu/hw/ide.c
> +++ b/qemu/hw/ide.c
> @@ -1717,6 +1717,7 @@ static void ide_atapi_cmd(IDEState *s)
>     case GPCMD_GET_CONFIGURATION:
>         {
>             uint32_t len;
> +            uint8_t index = 0;
>
>             /* only feature 0 is supported */
>             if (packet[2] != 0 || packet[3] != 0) {
> @@ -1753,14 +1754,11 @@ static void ide_atapi_cmd(IDEState *s)
>             }
>
>             len = 8; /* header completed */
> -            if (max_len > len) {
> -                uint8_t index = 0;
>
> -                buf[10] = 0x02 | 0x01; /* persistent and current */
> -                len += 4; /* header */
> -                len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_DVD_ROM);
> -                len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_CD_ROM);
> -            }
> +            buf[10] = 0x02 | 0x01; /* persistent and current */
> +            len += 4; /* header */
> +            len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_DVD_ROM);
> +            len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_CD_ROM);
>             cpu_to_ube32(buf, len - 4); /* data length */
>
>             ide_atapi_cmd_reply(s, len, max_len);

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

Please ignore this patch. It is valid for KVM only.

On May 24, 2008, at 12:31 PM, Alexander Graf wrote:

> Is there anything particularly wrong with this patch? Does it break  
> any guest OSs I am not aware of?
> If not, please include it as it's a pretty obvious bug.
>
> Alex
>
> On May 21, 2008, at 4:34 PM, Alexander Graf wrote:
>
>> The current implementation of GET_CONFIGURATION in the ATAPI stack  
>> of qemu replies a different length depending on the buffer, the  
>> data should be written into.
>>
>> On the other hand the SCSI spec defines that length information has  
>> to be consistent and independent of return buffer lengths.
>>
>> This patch makes the ATAPI emulation behave according to the spec  
>> and fixes the Darwin DVD driver.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Kevin Wolf <kwolf@suse.de>
>>
>>

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Carlo Marcelo Arenas Belon]

On Sat, May 24, 2008 at 12:31:01PM +0200, Alexander Graf wrote:
> Is there anything particularly wrong with this patch?

no, but the SPEC (I used MMC6 r01) says :

6.5.1.4 Allocation Length

The Allocation Length field specifies the maximum length in bytes of the Get
Configuration response data.  An Allocation Length field of zero indicates
that no data shall be transferred.  This condition shall not be considered an
error.

this is only implicitly supported by truncating the response using max_len
for the ide_atapi_cmd_reply with your patch and so the work of generating the
response is still happening regardless.

> Does it break any guest OSs I am not aware of?

the original implementation (*) broke [Open]Solaris by overflowing the buffer
that was used in the calls to detect the CDROM/DVDROM (using a 8 byte buffer)

Carlo

(*) http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

On May 25, 2008, at 9:53 AM, Carlo Marcelo Arenas Belon wrote:

> On Sat, May 24, 2008 at 12:31:01PM +0200, Alexander Graf wrote:
>> Is there anything particularly wrong with this patch?
>
> no, but the SPEC (I used MMC6 r01) says :
>
> 6.5.1.4 Allocation Length
>
> The Allocation Length field specifies the maximum length in bytes of  
> the Get
> Configuration response data.  An Allocation Length field of zero  
> indicates
> that no data shall be transferred.  This condition shall not be  
> considered an
> error.

This means that the Allocation Length field in the request specifies  
the maximum bytes of data to be returned. This does not affect the  
"Data Length" field in the response, which indicates the maximum  
amount of data available.

> this is only implicitly supported by truncating the response using  
> max_len
> for the ide_atapi_cmd_reply with your patch and so the work of  
> generating the
> response is still happening regardless.

Isn't io_buffer memcpy()ed by at most max_len in the end to the DMA  
buffer of the target? Bytes after max_len should not even be written  
to target OS memory.

>> Does it break any guest OSs I am not aware of?
>
> the original implementation (*) broke [Open]Solaris by overflowing  
> the buffer
> that was used in the calls to detect the CDROM/DVDROM (using a 8  
> byte buffer)

I don't see how that could break anything. The normal way of receiving  
a dynamic length packet with SCSI is as follows:

- Send a request with an Allocation Length of 4 (or slightly more) bytes
- Receive the response header, which includes the "Data Length" field.  
Now the OS knows the full length of the response packet
- Send a request with an Allocation Length of "Data Length" bytes
- Receive the full packet

I am pretty sure Solaris does the same. If anything breaks, it's a bug  
in the implementation of how buf gets written into the client memory.

Furthermore I tried to issue the same command on a real DVD Drive.  
There is a handy tool in Linux called "sg_raw". Using that you can  
just issue raw SCSI commands to your device. I received the following  
results:

  # sg_raw -vv -r 0x00 /dev/sg1 46 00 00 00 00 00 00 00 00 00
open /dev/sg1 with flags=0x802
SCSI Status: Good

Sense Information:
sense buffer empty

No data received

# sg_raw -vv -r 0x04 /dev/sg1 46 00 00 00 00 00 00 00 04 00
open /dev/sg1 with flags=0x802
SCSI Status: Good

Sense Information:
sense buffer empty

Received 4 bytes of data:
  00     00 00 00 88                                         ....

# sg_raw -vv -r 0x08 /dev/sg1 46 00 00 00 00 00 00 00 08 00
open /dev/sg1 with flags=0x802
SCSI Status: Good

Sense Information:
sense buffer empty

Received 8 bytes of data:
  00     00 00 00 88 00 00 00 10                             ........

# sg_raw -vv -r 0x88 /dev/sg1 46 00 00 00 00 00 00 00 88 00
open /dev/sg1 with flags=0x802
SCSI Status: Good

Sense Information:
sense buffer empty

Received 136 bytes of data:
  00     00 00 00 88 00 00 00 10  00 00 03 08 00 10 01  
00    ................
  10     00 08 00 00 00 01 03 04  00 00 00 02 00 02 03  
04    ................
  20     00 00 00 00 00 03 03 04  29 00 00 00 00 10 01  
08    ........).......
  30     00 00 08 00 00 10 01 00  00 1d 00 00 00 1e 04  
04    ................
  40     03 00 00 00 00 1f 01 00  00 2a 00 04 00 01 00  
00    .........*......
  50     00 2b 00 04 00 00 00 00  00 3b 01 04 00 00 00 00    . 
+.......;......
  60     01 00 03 00 01 03 00 04  03 00 01 00 01 04 03  
00    ................
  70     01 05 03 00 01 06 04 04  00 00 00 01 01 07 0d  
04    ................
  80     0c 00 00 00 01 0b 04 04                             ........


As you can see the "Data Length" field in the response is constantly  
the maximum bytes available as response.

Alex

>
>
> Carlo
>
> (*) http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161
>
>

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Carlo Marcelo Arenas Belon]

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

Alexander

sorry if I wasn't clear enough.  I wasn't arguing about your patch (which is
correct) but was trying to explain why the old implementation was done the way
it was, in an (obviously excessive) effort to prevent a buffer overflow for the
response (which used to be hardcoded to 32 bytes regardless of the size of the
buffer) as shown by :

  http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161

a slightly modified version of your patch (which I'd been using against kvm 
and validated correct with Linux and Solaris guests) attached.

the only difference, is that it handles explicitly the empty buffer case and
cleans up the len calculation which shouldn't had been calculated
conditionally as you pointed out.

Carlo

[-- Attachment #2: kvm-69-qemu-ide-dvdrom.patch --]
[-- Type: text/plain, Size: 3610 bytes --]

--- kvm-69/qemu/hw/ide.c        2008-05-12 04:30:43.000000000 -0700
+++ kvm-69/qemu/hw/ide.c        2008-05-25 01:44:27.000000000 -0700
@@ -1716,6 +1716,7 @@
     case GPCMD_GET_CONFIGURATION:
         {
             uint32_t len;
+            uint8_t index = 0;
 
             /* only feature 0 is supported */
             if (packet[2] != 0 || packet[3] != 0) {
@@ -1726,41 +1727,40 @@
 
             /* XXX: could result in alignment problems in some architectures */
             max_len = ube16_to_cpu(packet + 7);
-            /*
-             * XXX: avoid overflow for io_buffer if max_len is bigger than the
-             *      size of that buffer (dimensioned to max number of sectors
-             *      to transfer at once)
-             *
-             *      Only a problem if the feature/profiles grow exponentially.
-             */
-            if (max_len > 512) /* XXX: assume 1 sector */
-                max_len = 512;
-
-            memset(buf, 0, max_len);
-            /* 
-             * the number of sectors from the media tells us which profile
-             * to use as current.  0 means there is no media
-             *
-             * XXX: fails to detect correctly DVDs with less data burned
-             *      than what a CD can hold
-             */
-            if ((s -> nb_sectors)) {
-                if ((s -> nb_sectors > CD_MAX_SECTORS))
-                    cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
-                else
-                    cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
-            }
 
-            len = 8; /* header completed */
-            if (max_len > len) {
-                uint8_t index = 0;
-
-                buf[10] = 0x02 | 0x01; /* persistent and current */
-                len += 4; /* header */
-                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
-                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
-            }
-            cpu_to_ube32(buf, len - 4); /* data length */
+            if (max_len > 0) {
+                /*
+                 * XXX: avoid overflow for io_buffer if max_len is bigger than
+                 *      the size of that buffer (dimensioned to max number of
+                 *      sectors to transfer at once)
+                 *
+                 *      Only a problem if the feature/profiles grow 
+                 */
+                if (max_len > 512) /* XXX: assume 1 sector */
+                    max_len = 512;
+
+                memset(buf, 0, max_len);
+                /* 
+                 * the number of sectors from the media tells us which profile
+                 * to use as current.  0 means there is no media
+                 *
+                 * XXX: fails to detect correctly DVDs with less data burned
+                 *      than what a CD can hold
+                 */
+                if (s -> nb_sectors) {
+                    if (s -> nb_sectors > CD_MAX_SECTORS)
+                        cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
+                    else
+                        cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
+                }
+
+                buf[10] = 0x02 | 0x01; /* persistent and current */
+                len = 12; /* headers: 8 + 4 */
+                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
+                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
+                cpu_to_ube32(buf, len - 4); /* data length */
+            } else
+                len = 0;
 
             ide_atapi_cmd_reply(s, len, max_len);
             break;

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

On May 25, 2008, at 12:38 PM, Carlo Marcelo Arenas Belon wrote:

> Alexander
>
> sorry if I wasn't clear enough.  I wasn't arguing about your patch  
> (which is
> correct) but was trying to explain why the old implementation was  
> done the way
> it was, in an (obviously excessive) effort to prevent a buffer  
> overflow for the
> response (which used to be hardcoded to 32 bytes regardless of the  
> size of the
> buffer) as shown by :
>
>  http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161
>
> a slightly modified version of your patch (which I'd been using  
> against kvm
> and validated correct with Linux and Solaris guests) attached.
>
> the only difference, is that it handles explicitly the empty buffer  
> case and
> cleans up the len calculation which shouldn't had been calculated
> conditionally as you pointed out.
>
> Carlo
> <kvm-69-qemu-ide-dvdrom.patch>

Acked-by: Alexander Graf <agraf@suse.de>

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

On May 25, 2008, at 12:38 PM, Carlo Marcelo Arenas Belon wrote:

> Alexander
>
> sorry if I wasn't clear enough.  I wasn't arguing about your patch  
> (which is
> correct) but was trying to explain why the old implementation was  
> done the way
> it was, in an (obviously excessive) effort to prevent a buffer  
> overflow for the
> response (which used to be hardcoded to 32 bytes regardless of the  
> size of the
> buffer) as shown by :
>
>  http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161
>
> a slightly modified version of your patch (which I'd been using  
> against kvm
> and validated correct with Linux and Solaris guests) attached.
>
> the only difference, is that it handles explicitly the empty buffer  
> case and
> cleans up the len calculation which shouldn't had been calculated
> conditionally as you pointed out.
>
> Carlo
> <kvm-69-qemu-ide-dvdrom.patch>


Hum ... it might be a good idea to inline patches so commenting on  
them is easier.
You don't need to check for max_len > 0, because if max_len == 0 the  
packet will be truncated back to 0 bytes by ide_atapi_cmd_reply(s,  
len, max_len).

Alex

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Carlo Marcelo Arenas Belon]

On Mon, May 26, 2008 at 07:46:10PM +0200, Alexander Graf wrote:
> 
> On May 25, 2008, at 12:38 PM, Carlo Marcelo Arenas Belon wrote:
> 
> >a slightly modified version of your patch (which I'd been using  
> >against kvm
> >and validated correct with Linux and Solaris guests) attached.
> >
> >the only difference, is that it handles explicitly the empty buffer  
> >case and
> >cleans up the len calculation which shouldn't had been calculated
> >conditionally as you pointed out.
> >
> >Carlo
> ><kvm-69-qemu-ide-dvdrom.patch>
> 
> Hum ... it might be a good idea to inline patches so commenting on  
> them is easier.

patch was attached inline just like you did with your original one (except it
was using MIME type text/plain instead of text/x-patch).  inlining it
below so it is easier to comment on.

> You don't need to check for max_len > 0, because if max_len == 0 the  
> packet will be truncated back to 0 bytes by ide_atapi_cmd_reply(s,  
> len, max_len).

yes, the check is not needed but as I said was added so that it can
explicitly handle those cases for clarity and to save the work of building
the response just to be discarded at the end.  anyway, removed from the last
version which should be also ready for commit.

Carlo

---
Index: hw/ide.c
===================================================================
--- hw/ide.c	(revision 4589)
+++ hw/ide.c	(working copy)
@@ -1715,6 +1715,7 @@
     case GPCMD_GET_CONFIGURATION:
         {
             uint32_t len;
+            uint8_t index = 0;
 
             /* only feature 0 is supported */
             if (packet[2] != 0 || packet[3] != 0) {
@@ -1725,12 +1726,13 @@
 
             /* XXX: could result in alignment problems in some architectures */
             max_len = ube16_to_cpu(packet + 7);
+
             /*
-             * XXX: avoid overflow for io_buffer if max_len is bigger than the
-             *      size of that buffer (dimensioned to max number of sectors
-             *      to transfer at once)
+             * XXX: avoid overflow for io_buffer if max_len is bigger than
+             *      the size of that buffer (dimensioned to max number of
+             *      sectors to transfer at once)
              *
-             *      Only a problem if the feature/profiles grow exponentially.
+             *      Only a problem if the feature/profiles grow 
              */
             if (max_len > 512) /* XXX: assume 1 sector */
                 max_len = 512;
@@ -1743,22 +1745,17 @@
              * XXX: fails to detect correctly DVDs with less data burned
              *      than what a CD can hold
              */
-            if ((s -> nb_sectors)) {
-                if ((s -> nb_sectors > CD_MAX_SECTORS))
+            if (s -> nb_sectors) {
+                if (s -> nb_sectors > CD_MAX_SECTORS)
                     cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
                 else
                     cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
             }
 
-            len = 8; /* header completed */
-            if (max_len > len) {
-                uint8_t index = 0;
-
-                buf[10] = 0x02 | 0x01; /* persistent and current */
-                len += 4; /* header */
-                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
-                len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
-            }
+            buf[10] = 0x02 | 0x01; /* persistent and current */
+            len = 12; /* headers: 8 + 4 */
+            len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
+            len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
             cpu_to_ube32(buf, len - 4); /* data length */
 
             ide_atapi_cmd_reply(s, len, max_len);

Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Alexander Graf]

On May 27, 2008, at 8:10 PM, Carlo Marcelo Arenas Belon wrote:

> On Mon, May 26, 2008 at 07:46:10PM +0200, Alexander Graf wrote:
>>
>> On May 25, 2008, at 12:38 PM, Carlo Marcelo Arenas Belon wrote:
>>
>>> a slightly modified version of your patch (which I'd been using
>>> against kvm
>>> and validated correct with Linux and Solaris guests) attached.
>>>
>>> the only difference, is that it handles explicitly the empty buffer
>>> case and
>>> cleans up the len calculation which shouldn't had been calculated
>>> conditionally as you pointed out.
>>>
>>> Carlo
>>> <kvm-69-qemu-ide-dvdrom.patch>
>>
>> Hum ... it might be a good idea to inline patches so commenting on
>> them is easier.
>
> patch was attached inline just like you did with your original one  
> (except it
> was using MIME type text/plain instead of text/x-patch).  inlining it
> below so it is easier to comment on.
>
>> You don't need to check for max_len > 0, because if max_len == 0 the
>> packet will be truncated back to 0 bytes by ide_atapi_cmd_reply(s,
>> len, max_len).
>
> yes, the check is not needed but as I said was added so that it can
> explicitly handle those cases for clarity and to save the work of  
> building
> the response just to be discarded at the end.  anyway, removed from  
> the last
> version which should be also ready for commit.
>
> Carlo
>
> ---
> Index: hw/ide.c
> ===================================================================
> --- hw/ide.c	(revision 4589)
> +++ hw/ide.c	(working copy)
> @@ -1715,6 +1715,7 @@
>     case GPCMD_GET_CONFIGURATION:
>         {
>             uint32_t len;
> +            uint8_t index = 0;
>
>             /* only feature 0 is supported */
>             if (packet[2] != 0 || packet[3] != 0) {
> @@ -1725,12 +1726,13 @@
>
>             /* XXX: could result in alignment problems in some  
> architectures */
>             max_len = ube16_to_cpu(packet + 7);
> +
>             /*
> -             * XXX: avoid overflow for io_buffer if max_len is  
> bigger than the
> -             *      size of that buffer (dimensioned to max number  
> of sectors
> -             *      to transfer at once)
> +             * XXX: avoid overflow for io_buffer if max_len is  
> bigger than
> +             *      the size of that buffer (dimensioned to max  
> number of
> +             *      sectors to transfer at once)
>              *
> -             *      Only a problem if the feature/profiles grow  
> exponentially.
> +             *      Only a problem if the feature/profiles grow
>              */
>             if (max_len > 512) /* XXX: assume 1 sector */
>                 max_len = 512;
> @@ -1743,22 +1745,17 @@
>              * XXX: fails to detect correctly DVDs with less data  
> burned
>              *      than what a CD can hold
>              */
> -            if ((s -> nb_sectors)) {
> -                if ((s -> nb_sectors > CD_MAX_SECTORS))
> +            if (s -> nb_sectors) {
> +                if (s -> nb_sectors > CD_MAX_SECTORS)
>                     cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
>                 else
>                     cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
>             }
>
> -            len = 8; /* header completed */
> -            if (max_len > len) {
> -                uint8_t index = 0;
> -
> -                buf[10] = 0x02 | 0x01; /* persistent and current */
> -                len += 4; /* header */
> -                len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_DVD_ROM);
> -                len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_CD_ROM);
> -            }
> +            buf[10] = 0x02 | 0x01; /* persistent and current */
> +            len = 12; /* headers: 8 + 4 */
> +            len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_DVD_ROM);
> +            len += ide_atapi_set_profile(buf, &index,  
> MMC_PROFILE_CD_ROM);
>             cpu_to_ube32(buf, len - 4); /* data length */
>
>             ide_atapi_cmd_reply(s, len, max_len);
>
>

Looks fine to me. Please commit this patch or comment on it if you  
think it should not be included. As Paul is the one in the Maintainers  
file listed as SCSI maintainer, I would also love to hear a comment on  
this from him.

Thank you,

Alex