Fix ATAPI GET_CONFIGURATION function

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 <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

qemu/hw/ide.c +5 -7

index a2a8299..6e585e1 100644
@@ -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);

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

Quoted 10 lines · Alexander Graf

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 <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

Is there anything particularly wrong with this patch?

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

Quoted 1 line · Alexander Graf

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.

Quoted 1 line

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

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>

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>

Quoted 9 lines · Carlo Marcelo Arenas Belon

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.

Quoted 2 lines

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).

Quoted 12 lines · Alexander Graf

Quoted 9 lines · Carlo Marcelo Arenas Belon

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

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
===================================================================

ide.c +1 -0

@@ -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);

ide.c +1 -1

+
            /*
-             * XXX: avoid overflow for io_buffer if max_len is  

bigger than the
- * size of that buffer (dimensioned to max number
of sectors

ide.c +1 -1

-             *      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

ide.c +1 -1

+             *      sectors to transfer at once)
             *
-             *      Only a problem if the feature/profiles grow  

exponentially.

ide.c +1 -0

+             *      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
*/

ide.c +2 -9

-            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);

ide.c +3 -1

-            }
+            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);