[Qemu-devel] [PATCH] fix sector overflow for scsi disks >1TB large

[Qemu-devel] [PATCH] fix sector overflow for scsi disks >1TB large

[Rik van Riel]

Sector numbers can overflow on a virtual scsi disk of over 1TB
in size.  Qemu's bdrv_read expects an int64_t, so fix the overflow
by going to that data type.

Also clip capacity to 2TB instead of returning "capacity modulo 2TB".

Signed-off-by: Rik van Riel <riel@redhat.com>

Index: trunk/hw/scsi-disk.c
===================================================================
--- trunk/hw/scsi-disk.c	(revision 6451)
+++ trunk/hw/scsi-disk.c	(working copy)
@@ -50,7 +50,7 @@
     /* ??? We should probably keep track of whether the data trasfer is
        a read or a write.  Currently we rely on the host getting it right.  */
     /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
-    int sector;
+    uint64_t sector;
     int sector_count;
     /* The amounnt of data in the buffer.  */
     int buf_len;
@@ -731,6 +731,9 @@
         /* Returned value is the address of the last sector.  */
         if (nb_sectors) {
             nb_sectors--;
+            /* Clip to 2TB, instead of returning capacity modulo 2TB. */
+            if (nb_sectors > UINT_MAX)
+                nb_sectors = UINT_MAX;
             outbuf[0] = (nb_sectors >> 24) & 0xff;
             outbuf[1] = (nb_sectors >> 16) & 0xff;
             outbuf[2] = (nb_sectors >> 8) & 0xff;

Re: [Qemu-devel] [PATCH] fix sector overflow for scsi disks >1TB large

[Jamie Lokier]

Rik van Riel wrote:
> @@ -50,7 +50,7 @@
>      /* ??? We should probably keep track of whether the data trasfer is

Not your fault, but I just noticed a typo: "trasfer" :-)

>          if (nb_sectors) {
>              nb_sectors--;
> +            /* Clip to 2TB, instead of returning capacity modulo 2TB. */
> +            if (nb_sectors > UINT_MAX)
> +                nb_sectors = UINT_MAX;
>              outbuf[0] = (nb_sectors >> 24) & 0xff;
>              outbuf[1] = (nb_sectors >> 16) & 0xff;
>              outbuf[2] = (nb_sectors >> 8) & 0xff;

Wouldn't it be clearer andd safer to say 0xffffffff here, or UINT32_MAX?

I know QEMU only runs on hosts with 32-bit unsigned int, and perhaps
that will always be truea, but it's a bit unnecessary to assume it here.

-- JAmie

Re: [Qemu-devel] [PATCH] fix sector overflow for scsi disks >1TB large

[Rik van Riel]

Jamie Lokier wrote:
> Rik van Riel wrote:
>> @@ -50,7 +50,7 @@
>>      /* ??? We should probably keep track of whether the data trasfer is
> 
> Not your fault, but I just noticed a typo: "trasfer" :-)
> 
>>          if (nb_sectors) {
>>              nb_sectors--;
>> +            /* Clip to 2TB, instead of returning capacity modulo 2TB. */
>> +            if (nb_sectors > UINT_MAX)
>> +                nb_sectors = UINT_MAX;
>>              outbuf[0] = (nb_sectors >> 24) & 0xff;
>>              outbuf[1] = (nb_sectors >> 16) & 0xff;
>>              outbuf[2] = (nb_sectors >> 8) & 0xff;
> 
> Wouldn't it be clearer andd safer to say 0xffffffff here, or UINT32_MAX?
> 
> I know QEMU only runs on hosts with 32-bit unsigned int, and perhaps
> that will always be truea, but it's a bit unnecessary to assume it here.

Neither of your suggested changes hurts one bit, so
I'll send in a new patch :)

-- 
All rights reversed.

[Qemu-devel] [PATCH] fix sector overflow for scsi disks >1TB large

[Rik van Riel]

Sector numbers can overflow on a virtual scsi disk of over 1TB
in size.  Qemu's bdrv_read expects an int64_t, so fix the overflow
by going to that data type.

Also clip capacity to 2TB instead of returning capacity modulo 2TB.

Signed-off-by: Rik van Riel <riel@redhat.com>


Index: trunk/hw/scsi-disk.c
===================================================================
--- trunk/hw/scsi-disk.c	(revision 6451)
+++ trunk/hw/scsi-disk.c	(working copy)
@@ -47,10 +47,10 @@
 typedef struct SCSIRequest {
     SCSIDeviceState *dev;
     uint32_t tag;
-    /* ??? We should probably keep track of whether the data trasfer is
+    /* ??? We should probably keep track of whether the data transfer is
        a read or a write.  Currently we rely on the host getting it right.  */
     /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
-    int sector;
+    uint64_t sector;
     int sector_count;
     /* The amounnt of data in the buffer.  */
     int buf_len;
@@ -731,6 +731,9 @@
         /* Returned value is the address of the last sector.  */
         if (nb_sectors) {
             nb_sectors--;
+            /* Clip to 2TB, instead of returning capacity modulo 2TB. */
+            if (nb_sectors > UINT32_MAX)
+                nb_sectors = UINT32_MAX;
             outbuf[0] = (nb_sectors >> 24) & 0xff;
             outbuf[1] = (nb_sectors >> 16) & 0xff;
             outbuf[2] = (nb_sectors >> 8) & 0xff;